search

saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

Further synchronization #124

Closed ovalseven8 closed 5 years ago

ovalseven8 commented 5 years ago

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

dbrgn commented 5 years ago

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

Keep https://github.com/saltyrtc/saltyrtc-client-java/issues/114 in mind! So only things need to be synchronized that are being accessed concurrently within the library itself.

ovalseven8 commented 5 years ago

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?

ovalseven8 commented 5 years ago

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 commented 5 years ago

The latter.