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

Sys_error("Broken pipe") printed to console on opam list #4216

Closed
craigfe opened this issue Jun 2, 2020 · 6 comments · Fixed by #4901
Closed

Sys_error("Broken pipe") printed to console on opam list #4216

craigfe opened this issue Jun 2, 2020 · 6 comments · Fixed by #4901
Assignees
Projects
Milestone

Comments

@craigfe
Copy link

craigfe commented Jun 2, 2020

On 2.1.0~alpha, piping opam list into a process that breaks the pipe causes a "Fatal error" message to be printed to the console. The exit code is 0 regardless.

ᐅ opam list | head -n 5
# Packages matching: installed
# Name                  # Installed  # Synopsis
asetmap                 0.8.1        Alternative, compatible, OCaml standard library Sets and Maps
astring                 0.8.3        Alternative String module for OCaml
base                    v0.13.2      Full standard library replacement for OCaml
Fatal error: exception Sys_error("Broken pipe")

ᐅ echo $?
0

I would expect opam to handle a SIGPIPE here with no error message.

@xavierleroy xavierleroy transferred this issue from ocaml/ocaml Jun 2, 2020
@xavierleroy
Copy link

This issue was initially submitted on the ocaml/ocaml/ bug tracker, but it's really related to how OPAM chooses to handle SIGPIPE.

@craigfe
Copy link
Author

craigfe commented Jun 2, 2020

Indeed, apologies. Thanks for taking care of it.

@AltGr
Copy link
Member

AltGr commented Jun 2, 2020

Ha, funnily enough, the reason the error code is 0 is that in this case the exception happens in the finaliser, which runs flush stdout — after the main try with and the exit code has been decided.
So the good news is that this wouldn't happen if anything actually failed because of the broken pipe.

I am not sure what the better fix is: ensure to flush earlier ? Or just ignore the error once the main is done ? I guess that in this case, it would be best to exit with 0 and no error...

@avsm
Copy link
Member

avsm commented Jun 18, 2020

I think we should flush earlier before hitting the finaliser. We should avoid doing anything in the finalisers that might potentially raise. Also in this case, the flush stdout is a core part of what the point of the command is -- to list packages. That doesn't seem like a task for a finaliser :-)

@kit-ty-kate
Copy link
Member

This issue is still there in 2.1.0~rc2

@dra27 dra27 added this to To do in Opam 2.2.0 via automation Jul 8, 2021
@AltGr
Copy link
Member

AltGr commented Nov 9, 2021

From my tests the return value is actually 2, which is what OCaml returns by default on uncaught exceptions ?

(you need to set -o pipefail or run ( ./opam list; echo $? >&2 ) | head -n 5 instead or you'll get the exit code from head)

AltGr added a commit to OCamlPro/opam that referenced this issue Nov 9, 2021
Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
AltGr added a commit to OCamlPro/opam that referenced this issue Nov 9, 2021
Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
@rjbou rjbou moved this from To do to In progress in Opam 2.2.0 Nov 16, 2021
@rjbou rjbou added this to the 2.2.0~alpha milestone Nov 16, 2021
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this issue Jan 22, 2022
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this issue Jan 22, 2022
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this issue Jan 25, 2022
kit-ty-kate added a commit that referenced this issue Jan 26, 2022
Add some unix-only cram tests to test for #4216 before fixing it
AltGr added a commit to OCamlPro/opam that referenced this issue Jan 26, 2022
Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
AltGr added a commit to AltGr/opam that referenced this issue Jan 26, 2022
Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
AltGr added a commit to AltGr/opam that referenced this issue Jan 26, 2022
(from @kit_ty_kate)
AltGr added a commit to AltGr/opam that referenced this issue Jan 26, 2022
AltGr added a commit to AltGr/opam that referenced this issue Jan 26, 2022
AltGr added a commit to AltGr/opam that referenced this issue Jan 26, 2022
AltGr added a commit to AltGr/opam that referenced this issue Jan 28, 2022
rjbou pushed a commit to AltGr/opam that referenced this issue Jan 31, 2022
rjbou pushed a commit to AltGr/opam that referenced this issue Feb 1, 2022
rjbou pushed a commit to OCamlPro/opam that referenced this issue Feb 2, 2022
Restore the sigpipe handler before exiting, to prevent the finaliser
from crashing on flushes.

Closes ocaml#4216
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging a pull request may close this issue.

6 participants