search

saltyrtc / saltyrtc-client-java

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

Synchronize on Signaling class #125

Open ovalseven8 opened 5 years ago

ovalseven8 commented 5 years ago

Before that change, the handler methods for the WebSocket library were synchronized on the WebSocketAdapter while other methods in Signaling class were synchronized on the class itself.

Also, the public SaltyRTC API is also synchronized on the Signaling class now. With this change, the WebSocket library and the public API are synchronized on the same class which should guarantee thread-safety.

lgrahl commented 5 years ago

Thanks, this should do the trick for now.

Oh je, die Synchronized-Seuche... :crying_cat_face:

ovalseven8 commented 5 years ago

Oh je, die Synchronized-Seuche... :crying_cat_face:

Yeah, it's not perfect. However, after looking at the code other solutions would result in major refactorings, especially in the Signaling class and encapsulation of the websocket handling. At least, I could not find another way...

So, for the time being, this should at least improve the current situation. As there is a public API, some synchronized blocks are needed nonetheless. Another way would be more of a actor-like approach (see Akka), what means a complete restructure of the code.

lgrahl commented 5 years ago

I think we need to carefully investigate and ensure this doesn't deadlock anything in Threema.

lgrahl commented 5 years ago

What is it with you closing PRs? :wink: