concurrency bug fixes/ improvements #4663
Conversation
This is the right thing to do because othewise it is not possible to create new ideStates in a single instance of the executable. This will be useful if the hls executable is supposed to talk to multiple clients and lives beyond a single client disconnecting.
Previously, when there was no shutdown message by a client and the client disconnected, resulting in the handlers to be GC'd the race that was supposed to free resources for the HieDB & co. would throw a hard error talking about the MVar being unreachable. We would like to instead finish gracefully because finishing the race as soon as the MVar was GC'd is the right thing to do anyway.
| -- Rethrows any exceptions. | ||
| untilMVar :: MonadUnliftIO m => MVar () -> m () -> m () | ||
| untilMVar mvar io = void $ | ||
| waitAnyCancel =<< traverse async [ io , readMVar mvar ] | ||
| untilMVar mvar io = race_ (readMVar mvar `catch` \BlockedIndefinitelyOnMVar -> pure ()) io |
There was a problem hiding this comment.
This seems a little bit round-about, is this really preferable over
| untilMVar mvar io = race_ (readMVar mvar `catch` \BlockedIndefinitelyOnMVar -> pure ()) io | |
| untilMVar mvar io = race_ (readMVar mvar) (io `finally` putMVar mvar ()) |
Also, I am not quite sure if I understand whether the io thread dying without putting the MVar is a bug on its own that needs fixing? What does dying mean here, does the thread crash for some reason?
There was a problem hiding this comment.
The io is not the thing putting the MVar. The MVar is put my the shutdown notification. If none is sent but the connection dies anyway then the MVar gets GC'd and the thread that tries to read the MVar gets a blocked indefinitely on MVar exception. Thats why your proposed change wouldn't work.
There was a problem hiding this comment.
The "real" fix is to put the MVar as a bracket around the server dying, too.
There was a problem hiding this comment.
Maybe the server shouldn't crash but gracefully shutdown if the connection is dropped?
There was a problem hiding this comment.
Im not sure it crashes. It may just be that the thread dies before it receives a shutdown notification. That's very well possible if the client doesn't/ can't implement graceful shutdown
There was a problem hiding this comment.
But the server can handle the connection termination gracefully, right?
There was a problem hiding this comment.
Yea. The thread just drops. Which makes the putMVar drop, too. Which triggers a threadBlockedIndefinitelyOnMVar exception
There was a problem hiding this comment.
Right, but can we avoid relying on the rts for this case?
There was a problem hiding this comment.
Well I'm assuming that the io will also finish so it's not a problem to not rely on the RTS, perhaps it just finishes later most of the time. But that's what I outlined above. The ideal fix is to hand the MVar somewhere else where it can be put as part of some bracketing operation. But since that ideal fix doesn't give us any but conceptual advantages, idk if it's necessary for now
|
The second commit seems to be neither a bug fix nor a concurrency improvement. |
|
It doesn't appear to be one as of now ;) Later when we create multiple clients per run of the executable, it's important that we can create multiple ide states, too. |
|
Right, as of now it is a random change :P As you know, I don't think the complexity that might be introduced by handling multiple clients at the same time should be handled within HLS. Perhaps if the complexity was encapsulated in a separate module / executable. |
|
Well this complexity is necessary if we ever want a single executable with multiple clients which I think is actually required to make it feasible at all, mainly wrt memory footprint. |
|
I'll create a ticket. |
|
@fendor so whether or not I create the ticket, are these changes controversial? |
|
No, these changes are not controversial |
Hi. I have two concurrency bug fixes/ improvements that pave the way towards multi client haskell-language-server.
[fix] don't bake ide state mvar into setup and getIdeState
This is the right thing to do because othewise it is not possible to
create new ideStates in a single instance of the executable. This will
be useful if the hls executable is supposed to talk to multiple clients
and lives beyond a single client disconnecting
[fix] don't throw hard errors when no shutdown message is handled
Previously, when there was no shutdown message by a client and the
client disconnected, resulting in the handlers to be GC'd the race that
was supposed to free resources for the HieDB & co. would throw a hard
error talking about the MVar being unreachable. We would like to instead
finish gracefully because finishing the race as soon as the MVar was
GC'd is the right thing to do anyway.