Make output_stream_t::append() fallible and handle SIGINT on append
#9266
Conversation
mqudsi
force-pushed
the
fallible_append
branch
from
a7f5a95 to
31c2c78
Compare
| // output stream as errored, causing us to both return false and skip any future writes. | ||
| // We're currently going with the latter, especially seeing as no callers currently | ||
| // check the result of `append()` (since it was always a void function before). | ||
| } else if (errno != EPIPE) { | ||
| wperror(L"write"); | ||
| } | ||
| errored_ = true; |
There was a problem hiding this comment.
Are there any cases where we *shouldn'*t error?
E.g. if we get EINTR for other signals? EAGAIN?
This now is quite eager to stop writing and I'm scared.
There was a problem hiding this comment.
This now is quite eager to stop writing and I'm scared.
The behavior hasn't changed. It always set errored_ = true; in case of EINTR but now we just don't call wperror(..) before doing so.
There was a problem hiding this comment.
Huh, you're right, now I'm confused as to why I've never seen an error for EINTR being printed here.
Allow errors encountered by certain implementations of `output_stream_t` when writing to the output sink to be bubbled back to the caller.
mqudsi
force-pushed
the
fallible_append
branch
from
31c2c78 to
bc94e53
Compare
If EINTR caused by SIGINT is encountered while writing to the `fd_output_stream_t` output fd, mark the output stream as errored and return false to the caller but do not visibly complain. Addressing the outstanding TODO notwithstanding, this is needed to avoid littering the tty with spurious errors when the user hits Ctrl-C to abort a long-running builtin's output (w/ the primary example being `history`).
When there are multiple screens worth of output and `history` is writing to the
pager, pressing Ctrl-C at the end of a screen doesn't exit the pager (`q` is
needed for that) but previously caused fish to emit an error ("write:
Interrupted system call) until we starting silently handling SIGINT in
`fd_output_stream_t::append()`.
This patch makes `history` detect when the `append()` call returns with an error
and causes it to end early rather than repeatedly trying (and failing) to write
to the output stream.
mqudsi
force-pushed
the
fallible_append
branch
from
bc94e53 to
a9d5690
Compare
| // the return value and skip future calls to `append()`) or we can flag the entire | ||
| // output stream as errored, causing us to both return false and skip any future writes. | ||
| // We're currently going with the latter, especially seeing as no callers currently | ||
| // check the result of `append()` (since it was always a void function before). |
There was a problem hiding this comment.
we can flag the entire output stream as errored, causing us to both return false and skip any future writes.
I guess for EINTR that's somewhat implied because SIGINT cancels builtin execution.
| } | ||
|
|
||
| // We are intentionally not returning false in case of an output error, as the user aborting the | ||
| // output early (the most common case) isn't a reason to exit w/ a non-zero status code. |
There was a problem hiding this comment.
.. but in this case we likely exit with a non-zero signal code like SIGINT = 130
| // Don't force an error if output was aborted (typically via Ctrl-C/SIGINT); just don't | ||
| // try writing any more. | ||
| output_error = true; | ||
| } |
There was a problem hiding this comment.
history is the worst offender (can't seem to hit the wperror("write") on Linux though).
There are probably more cases where we could stop writing early (string repeat) etc.
Anyway, this seems like a good step forward, LGTM!
This patch series addresses a long-standing issue (
output_stream_t::append()is assumed to never fail and returns void) that made it difficult to handle output termination via Ctrl-C (by halting progress and ending early) for various builtins with lots of output,historybeing the best example because it wrote to the pager (giving the user plenty of time and reason toCtrl-Cthe running process, but to no effect besides an error message).Make
output_stream_t::append()fallibleAllow errors encountered by certain implementations of
output_stream_twhenwriting to the output sink to be bubbled back to the caller.
Silently handle fd_output_stream_t append errors in case of SIGINT
If
EINTRcaused bySIGINTis encountered while writing to thefd_output_stream_toutput fd, mark the output stream as errored and returnfalse to the caller but do not visibly complain.
Addressing the outstanding TODO notwithstanding, this is needed to avoid
littering the tty with spurious errors when the user hits Ctrl-C to abort a
long-running builtin's output (w/ the primary example being
history).history: Handle Ctrl-C/SIGINT or other errors on output append
When there are multiple screens worth of output and
historyis writing to thepager, pressing Ctrl-C at the end of a screen doesn't exit the pager (
qisneeded for that) but previously caused fish to emit an error ("write:
Interrupted system call) until we started silently handling SIGINT in
fd_output_stream_t::append().This patch makes
historydetect when theappend()call returns with an errorand causes it to end early rather than repeatedly trying (and failing) to write
to the output stream.