Further synchronization

[ovalseven8]

After skimming through the code I believe further synchronization is necessary to avoid lost updates or other concurrency issues.

[dbrgn]

Thanks! I'll take a look tomorrow if I find time.

Keep #114 in mind! So only things need to be synchronized that are being accessed concurrently within the library itself.

[ovalseven8]

Some reasoning:
The callbacks are being executed by different threads in the current implementation (at least ReadingThread and WritingThread). The callbacks can run in parallel as far as I understand.

Why did I add synchronized inside onDisconnected() and other callbacks?
AFAIK it's possible that onDisconnected() is executed by at least both ReadingThread and WritingThread. So, without this change it's possible that onDisconnected() and onBinaryMessage() run in parallel. Both access shared state. Lost updates and race conditions should be possible.
That's also the reason why onConnected(), onConnectError() and onTextMessage() do need the synchronization IMHO.

Why did I add synchronized for onCallbackError()?
The method calls internally the synchronized method resetConnection(), however, resetConnection() is synchronized using the Signaling class instance while we also need synchronization on the WebSocketAdapter class instance. Because we have to guarantee a "happens before" relationship, only this way the JVM makes sure that the thread uses the latest changes on variables like this.state. Otherwise, "lost updates" are possible. I am not sure if we need synchronization for resetConnection() anymore then.

---

It's possible that some synchronization on other methods is redundant now. However, the callbacks need it I think?

[lgrahl]

I think your analysis is correct and we should merge this. One nit in-line.

That being said, we still face many potential race conditions since the application uses its own thread, so this only fixes potential races between the WebSocket threads. See #114 for details.

[ovalseven8]

That being said, we still face many potential race conditions since the application uses its own thread, so this only fixes potential races between the WebSocket threads.

You mean in combination with e.g. saltyrtc-task-webrtc-java? Or even in saltyrtc-client-java alone?

[lgrahl]

The latter.