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.

Sign up for GitHub

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

Jump to bottom

Ignore SIGPIPE in the Merlin server process #1746

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Ignore SIGPIPE in the Merlin server process #1746

merged 4 commits into from
Apr 19, 2024

Conversation

liam923
Copy link
Contributor

@liam923 liam923 commented Apr 4, 2024

Bringing in changes from merlin-jst.

From @catern:

The merlin server writes to pipes received from the merlin client process. If the read end of those pipes is closed, e.g. because the merlin client process was killed, then when the server writes to the pipe it will generate a SIGPIPE. This kills the Merlin server. Ignore the SIGPIPE instead of unnecessarily dying.

The fact that you get SIGPIPE when you write to a closed pipe is a classic Unix footgun, and this is the usual resolution. Another solution is to avoid pipes (in favor of e.g. socketpairs), but we can't do that because we write directly to the file descriptors received from the client.

voodoos
voodoos reviewed Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

The changes look reasonnable to me.

Just a few thougts to understand better why this change is useful:

Merlin's "client" processes are usually short lived and not subject to frequent crashes. (Crashes due to issues with analysis crash the server, not the client.)

So I guess this is most impactful when Merlin gets stuck into a slow computation and the user kills the client process to bring back control to the editor ? (This doesn't stop the server from finishing the work, and then crash due to sigpipe.)

Am I correct ? Is there another sequence of events that lead to these undesirable sigpipes ?

src/frontend/ocamlmerlin/ocamlmerlin_server.ml Outdated Show resolved Hide resolved
@catern
Copy link
Contributor

catern commented Apr 6, 2024

So I guess this is most impactful when Merlin gets stuck into a slow computation and the user kills the client process to bring back control to the editor ? (This doesn't stop the server from finishing the work, and then crash due to sigpipe.)

Yes, that's exactly right.

BTW, since killing the client process doesn't actually cancel the operation, subsequent merlin clients still have to wait for the first operation to complete. So control can be returned to the user, but subsequent commands may still have slow results. It would be nice if killing the client actually also cancelled the operation.

@voodoos
Copy link
Collaborator

voodoos commented Apr 19, 2024

Thanks @catern !

@voodoos voodoos merged commit 23e9de3 into ocaml:master Apr 19, 2024
12 checks passed
voodoos added a commit to voodoos/merlin that referenced this pull request May 13, 2024
@voodoos
[B] Ignore SIGPIPE in the Merlin server process (ocaml#1746)
9e9d1a5 
from liam923/ignore-sigpipe
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 17, 2024
@voodoos
[new release] merlin (3 packages) (5.0-502)
1fded17 
CHANGES:

Fri May 17 19:59:42 CET 2024

  + merlin binary
    - Support for OCaml 5.2 (ocaml/merlin#1757)
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
    - Addition of a `merlin-lib.commands` library which disassociates the
      execution of commands from the `new_protocol`, from the binary, allowing
      it to be invoked from other projects (ocaml/merlin#1758)
    - New occurrences backend: Don't index occurrences when `merlin.hide`
      attribute is present. (ocaml/merlin#1768)
    - Use the new `uid_to_decl` table in 5.2's cmt files to get documentation.
      (ocaml/merlin#1773)
@voodoos voodoos mentioned this pull request May 17, 2024
Merged
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 31, 2024
@voodoos
[new release] merlin (3 packages) (4.15-501)
2cf5ede 
CHANGES:

May May 31 14:02:42 CET 2024

  + merlin binary
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 31, 2024
@voodoos
[new release] merlin (3 packages) (4.15-414)
49ef452 
CHANGES:

Fri May 31 14:02:42 CEST 2024

  + merlin binary
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 31, 2024
@voodoos
[new release] merlin (3 packages) (4.15-501)
f6fdba8 
CHANGES:

Fri May 31 14:02:42 CEST 2024

  + merlin binary
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 31, 2024
@voodoos
[new release] merlin and merlin-lib v4.15-414/501
776ae45 
CHANGES:

Fri May 31 14:02:42 CEST 2024

  + merlin binary
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
@voodoos voodoos mentioned this pull request May 31, 2024
Merged
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
@voodoos @avsm
[new release] merlin and merlin-lib v4.15-414/501
c9ab3f5 
CHANGES:

Fri May 31 14:02:42 CEST 2024

  + merlin binary
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojb nojb nojb left review comments

@voodoos voodoos voodoos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Backport to 501
Status: 5.0-501
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liam923 @catern @voodoos @nojb