Skip to content

Conversation

@soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Aug 24, 2025

The main purpose of this patch is to fix the long standing broken pipe issue in hls, see haskell/haskell-language-server#1875. It accompanied with the hls fix haskell/haskell-language-server#4701

This pull request introduces several improvements to the LSP server's shutdown and exit handling. The changes ensure that output is properly flushed and errors are logged if the client disconnects unexpectedly, and that the server's sender thread is gracefully shut down. The shutdown and exit logic is now more explicit with new state tracking and barriers.

  • Added a new resExit barrier to LanguageContextState and an isExiting function to track when the server should exit, ensuring that the server stops processing messages after an exit notification is received. [1]
  • Enhanced clientOut to catch and log BrokenPipe errors when sending output fails (e.g., if the client closes the output handle), including a truncated version of the outgoing message for debugging.
  • Replaced manual thread management with withAsync and race to better manage the sender thread lifecycle and ensure clean shutdowns. [1]
  • Updated the sendServer function to terminate after sending the shutdown response, preventing further messages from being sent after shutdown. Modified the main server loop and sender logic to wait for the sender thread to finish after shutdown, with a timeout and logging if it does not stop in time.
  • exitNotificationHandler now do nothing.

summary generated by Copilot

@soulomoon soulomoon changed the title Gracefullyexit Gracefully exit the server Aug 24, 2025
@michaelpj
Copy link
Collaborator

Do you want a review on this yet?

@soulomoon
Copy link
Collaborator Author

Yes, the this MR is ready for review, but the corresponding hls changes at haskell/haskell-language-server#4701 still need some more polishing.

@soulomoon soulomoon marked this pull request as ready for review August 26, 2025 13:32
@soulomoon soulomoon marked this pull request as draft August 28, 2025 18:07
@soulomoon

This comment was marked as outdated.

@fendor
Copy link
Collaborator

fendor commented Nov 20, 2025

@soulomoon I assume, you still want a review? 🙈

I mean, this is still ready for review, right?

@fendor fendor requested review from fendor and michaelpj November 20, 2025 16:32
@fendor fendor self-assigned this Nov 20, 2025
@soulomoon
Copy link
Collaborator Author

soulomoon commented Nov 21, 2025

@fendor Yes, thanks for picking it up. If the general change looks good, I can resolve the conflict.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, got some nitpicks and mostly documentation update requests.
Thank you for chasing this issue down, it is really quite annoying!

Should we perhaps add a Note [Handle Server exiting] or something like that?

- Track shutdown/exit separately and wait for sender to flush before exit
- Make runServerWithHandles resilient to broken pipes and log truncated messages
- Have runServerWith/ioLoop return proper exit codes based on shutdown state
- Adjust processing to use sender wait action during shutdown and log exit
- Fix functional test harness to assert zero exit code instead of ExitSuccess
- Tidy minor CI workflow whitespace in format.yaml and nix.yaml
Co-authored-by: fendor <fendor@users.noreply.github.com>
@fendor fendor merged commit c4ca81b into haskell:master Nov 25, 2025
14 checks passed
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.

3 participants