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

can multiple threads send at the same time? #208

Open
dktr0 opened this issue Jun 25, 2020 · 13 comments
Open

can multiple threads send at the same time? #208

dktr0 opened this issue Jun 25, 2020 · 13 comments

Comments

@dktr0
Copy link

dktr0 commented Jun 25, 2020

Possibly a simple question but I'm having a hard time finding an answer for it: can 'send' be called with the same Connection from different threads concurrently? (Seeing a bug in my application that would be explained if the answer was 'no, not safely' but also can't find anywhere where it says this is safe or not safe...) Thanks!

@ondrap
Copy link
Contributor

ondrap commented Oct 8, 2020

I had the same problem and IMO the answer is probably 'no'. Which frightens me a little as e.g. the receiveData function can also send messages.
However, given that I am using per-message-deflate and the messages did arrive, it seems to me that the problem might be somewhere else - I would have expected a decompression error, not intermixed messages.

@ondrap
Copy link
Contributor

ondrap commented Oct 8, 2020

So after searching a little I suspect this to be a problem:

makeMessageDeflater (Just pmd)
    | serverNoContextTakeover pmd = do
        return $ \msg -> do
            ptr <- initDeflate pmd
            deflateMessageWith (deflateBody ptr) msg
    | otherwise = do
        ptr <- initDeflate pmd
        return $ \msg ->
            deflateMessageWith (deflateBody ptr) msg

Unless the connection is with serverNoContextTakeover, this is very likely thread unsafe. I do not easily see how this may be made thread-safe though; the messages must be sent in the exact order in which they were compressed, so the only way seems to me would be to do a wrapper around write in Network.WebSockets.Connection/acceptRequestWith. It seems to be safe with control messages, so it is probably possible to serialize this in the app and it shouldn't cause any problems.

@dktr0
Copy link
Author

dktr0 commented Oct 8, 2020

Thanks! In the months that have passed since I originally posted this issue, I refactored my application to not send from multiple threads and the problem seems to have gone away. Mentioning as further "evidence" in case it helps anyone else.

@kabuhr
Copy link

kabuhr commented Mar 2, 2022

@dktr0, can you confirm that you were using PermessageDeflateCompression in your app as well, when you noticed the issue?

@dktr0
Copy link
Author

dktr0 commented Mar 2, 2022

@kabuhr I'm not sure but I think I would have been using whatever the default connections are?

@qwbarch
Copy link

qwbarch commented Mar 3, 2022

Hmm so it's not from compression then? Since the default connection options doesn't have it enabled

@dktr0 How were you able to refactor your application to send messages from a single thread, when receiveData also sends messages (on pings and close)? I asked about this on stack overflow as well

Edit: Nevermind, took a look at your repository for inspiration. I guess it's not too much of a worry about receiveData sending messages from other threads then

@dktr0
Copy link
Author

dktr0 commented Mar 3, 2022

@qwbarch it might be a worry though still? I will say that we still do experience highly intermittent/refractory problems where a websocket connection seems to die (from the client's point of view) without ever dying from the server's point of view, and now this renewed conversation is making me wonder if it doesn't have something to do with this still...

@domenkozar
Copy link
Collaborator

@qwbarch it might be a worry though still? I will say that we still do experience highly intermittent/refractory problems where a websocket connection seems to die (from the client's point of view) without ever dying from the server's point of view, and now this renewed conversation is making me wonder if it doesn't have something to do with this still...

I'm seeing that too, did you ever manage to narrow it down?

@dktr0
Copy link
Author

dktr0 commented Jun 27, 2022

@domenkozar not yet - still on the TODO list...

@domenkozar
Copy link
Collaborator

@dktr0 do you have some notes on what happens? Does the ping thread help reconnect? On my side it doesn't so I'm a bit surprised.

@dktr0
Copy link
Author

dktr0 commented Jun 27, 2022

@domenkozar unfortunately I haven't had the chance to investigate further, so all I can really say about this is from the standpoint of observing what happens in a production deployment (of Estuary, a collaborative live coding platform https://github.com/dktr0/Estuary) when it is left running for a long time.

For example, currently the main deployment of this has been running continuously for 29 days and has handled just under 5000 sessions in that time. In the same time, the server seems to have accumulated about ~25 of these phantom connections. The application does have a ping thread, FWIW.

@domenkozar
Copy link
Collaborator

@dktr0 do you use a proper implementation of ping-pong? More in this thread: #159

@dktr0
Copy link
Author

dktr0 commented Jul 4, 2022

@domenkozar ah, I'm almost definitely not doing it right. Many thanks for this tip!

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

No branches or pull requests

5 participants