Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize on Signaling class #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ovalseven8
Copy link
Contributor

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.

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
Copy link
Member

lgrahl commented Jul 24, 2019

Thanks, this should do the trick for now.

Oh je, die Synchronized-Seuche... 😿

@lgrahl lgrahl requested a review from dbrgn July 24, 2019 08:17
@ovalseven8
Copy link
Contributor Author

ovalseven8 commented Jul 24, 2019

Oh je, die Synchronized-Seuche... 😿

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
Copy link
Member

lgrahl commented Jul 30, 2019

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

@ovalseven8 ovalseven8 closed this Sep 4, 2019
@lgrahl
Copy link
Member

lgrahl commented Sep 4, 2019

What is it with you closing PRs? 😉

@lgrahl lgrahl reopened this Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants