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

Fix synchronisation #114

Open
lgrahl opened this issue Jun 17, 2019 · 3 comments
Open

Fix synchronisation #114

lgrahl opened this issue Jun 17, 2019 · 3 comments

Comments

@lgrahl
Copy link
Member

lgrahl commented Jun 17, 2019

This library is not thread-safe and thus does not need synchronisation. All synchronized blocks and synchronised lists etc. should be removed/replaced.

Since the WebSocket implementation does use threads internally, we still need synchronisation between it and the public API surface. I see a couple of options:

  1. Make everything synchronized.
  2. Introduce an API to hand in a lock from the application that is being used for pretty much everything.
  3. Get rid of the WebSocket threads and allow this library to be run single-threaded (my preference, mid-term).
  4. Remove the WebSocket transport. Create a transport interface and let the application create its own WebSocket transport (my preference, long-term).
@lgrahl lgrahl changed the title Remove synchronisation Fix synchronisation Jul 18, 2019
@dbrgn
Copy link
Member

dbrgn commented Jul 18, 2019

I'd explore 3, potentially using a single thread executor service: https://github.com/TakahikoKawasaki/nv-websocket-client#connect-to-server-asynchronously

@ovalseven8
Copy link
Contributor

I'm not entirely sure what you mean, @dbrgn. Can you explain how you would structure that?

@ovalseven8
Copy link
Contributor

For the time being #125 basically introduces @lgrahl's 1./2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants