Add lock to database methods that edit the Jam state

[hwray]

Right now, we're experiencing some (infrequent) desync errors as a result of Clients making simultaneous modification requests to the Master server. We should wrap database methods that edit the Jam state with some kind of shared lock so that there can't be any desync errors due to concurrent modification of the database.

[hwray]

I tried adding synchronized to the methods in the DatabaseHandler that edit the Jam, but then one of the Client phones started failing to update its Jam in response to some updates by the other phones. Not sure what's going on here.

[hwray]

After some higher-level thinking, we really need to accomplish two things:

1. No two threads should ever be modifying the Jam at the same time. This can be accomplished through the use of synchronized keywords on DatabaseHandler methods that modify the Jam database.
2. Even with the above tactic, we still face issues. The Master phone could finish updating the Jam in response to one request (i.e. finish calling synchronized methods), then start sending the Jam state to JSON and broadcasting it to the Clients. Meanwhile, another Client request could begin concurrently modifying the DB on a background thread spawned by NanoHTTPD, while the Jam is being sent to JSON.

I think we can solve this by taking a "transactional" approach to the synchronization. One "transaction" will consist of the following:

• A Client request hits the Master phone's server /jam/{method} endpoint.
• The Master phone modifies its canonical version of the Jam to reflect the edit request.
• Lastly, the Master phone broadcasts the new Jam state to all clients. The Jam state must be stable, with no background threads editing concurrently, so it should be sent to JSON before the transaction lock is released.

Each "transaction" should acquire a single global lock, so that transactions are forced to proceed in a sequential manner.

@JayThomason, let me know what you think of this approach. Do you think it will help? Do you foresee any potential deadlock conditions with the introduction of a global lock on transactions?

[JayThomason]

Theoretically a transactional approach to jam modification is what we want. We have to be very careful that we can't get stuck in a transaction and that we always back out (undo any changes we have made) if there is an error.

You are right in saying that the when the jam is serialized to JSON it must be locked. The jam should be locked on action action involving the actual contents (songs) of the jam, whether the action involves reading or writing doesn't matter.

I think that using a lock on the jam is the right idea. I don't think that we necessarily have to add synchronized methods to the DatabaseHandler, because according to this the class we are extending with the DatabaseHandler only has one connection to the database and it is already using a lock to serialize reads/writes to the database.

Any methods on the jam that involve changing/reading the state of the songs should look like:

...
sychronized (this) {
 // read/write jam state
    data = getData();
}
return data;

[hwray]

Awesome Stack Overflow link, I had missed that in my understanding of the SQLiteOpenHelper class. I'll think a little more about the locking strategy and any potential deadlock conditions, then start trying to implement this.

[JayThomason]

I also think that if you synchronize on the server methods that would work.

[hwray]

Good call @JayThomason. I synchronized the server methods that update the Jam and everything seems to be staying in sync now, even after concurrent drag-and-drops from both clients. However, I suppose we still face the possibility of local edits running into conflict with an edit from the Server side, so we should still think about synchronizing the Jam or Database methods.

[hwray]

The Jam and Database methods are called from all over, often in succession, to accomplish a higher-level task that relies on results form previous calls. This means that simply synchronizing the methods at the object level won't work.

For now, I've added a global ReentrantLock jamLock, which now wraps all code blocks that update and rebroadcast the Jam state (changes made to: Server, BrowseJamFragment for setting and moving songs, BrowseSongsFragment for adding songs, BrowseMusicAdapter for removing songs). Things seem to working well - all 3 phones can execute drag-and-drop requests simultaneously without things getting out of sync.

However, I did encounter a deadlock on the Master phone at one point, and not during a time of a lot of simultaneous Client requests. I should wrap the unlock calls in a "try... finally" block to ensure we're always unlocking.

[hwray]

This is all fixed and working now. I wrapped all the jamLock code in try-finally's and we're no longer getting any deadlocks.

Summary of changes made to avoid errors and desync due to concurrent edits:

1. Added synchronized keyword to all Server methods that receive requests to update the Jam state.
2. Wrapped all blocks of code that update the Jam in blocking calls to a ReentrantLock jamLock. This code is located in BrowseMusicAdapter, BrowseSongsFragment, BrowseJamFragment, and Server (which probably renders the change in number 1 redundant).