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

handles the BrokenPipeError #101

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

christophkloeffel
Copy link
Contributor

fixes #94

@christophkloeffel christophkloeffel requested a review from a team as a code owner September 10, 2024 14:55
@christophkloeffel christophkloeffel enabled auto-merge (squash) September 10, 2024 14:57
trlc/trlc.py Outdated
@@ -40,6 +40,8 @@
except ImportError: # pragma: no cover
VCG_API_AVAILABLE = False

# Ignore SIGPIPE (Broken pipe) signal
signal.signal(signal.SIGPIPE, signal.SIG_DFL)
Copy link
Member

@phiwuu phiwuu Sep 10, 2024

Choose a reason for hiding this comment

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

While reviewing this I tried to figure out what exactly the "default behavior" is. The documentation is very nebulous about it. On the one hand, whatever the behavior is, it probably solves #94.
On the other hand, https://docs.python.org/3/library/signal.html#note-on-sigpipe recommends that one should not use SIG_DFL in this very scenario we have at hand here.

In order to come to a conclusion, we have to decide the following: What is the expected return code? Should we call sys.exit(1) or sys.exit(0) in a scenario where we run into BrokenPipeError?

See my comment in the related issue: #94 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, Python catches the SIGPIPE signal and raises a BrokenPipeError when it occurs. The signal.signal(signal.SIGPIPE, signal.SIG_DFL) line restores the default operating system behaviour for SIGPIPE, which is to terminate the program without raising an error. This is useful in cases like piping output to head, where you expect the pipe to be closed, and you don't want an error to be printed.

For my understanding Python is not executing the default behaviour of this signal https://docs.python.org/3/library/signal.html#signal.SIGPIPE, so I explicitly tell Python to do so. And not raise an Error.

@phiwuu phiwuu added bug Something isn't working topic: core Affects lexer/parser/infrastructure labels Sep 10, 2024
@phiwuu phiwuu changed the title ignores the BrokenPipeError ignores the BrokenPipeError Sep 10, 2024
@christophkloeffel christophkloeffel force-pushed the patch/broken-pipe-error branch 3 times, most recently from bd8e8b7 to 4ef9550 Compare September 11, 2024 09:25
trlc/trlc.py Outdated Show resolved Hide resolved
@christophkloeffel
Copy link
Contributor Author

christophkloeffel commented Sep 11, 2024

https://docs.python.org/3/library/signal.html#note-on-sigpipe using the exception handling provided as an example,
I now return 141 explicitly in case of a BrokenPipeError

@christophkloeffel christophkloeffel changed the title ignores the BrokenPipeError handles the BrokenPipeError Sep 11, 2024
@christophkloeffel christophkloeffel merged commit 86b114a into main Sep 11, 2024
16 checks passed
@christophkloeffel christophkloeffel deleted the patch/broken-pipe-error branch September 11, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working topic: core Affects lexer/parser/infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trlc crash when used in pipeline
2 participants