Propagate read errors in IO backend#7967
Merged
headius merged 3 commits intojruby:masterfrom Oct 18, 2023
Merged
Conversation
This can be improved in 11 by using select(Consumer<SelectionKey>) and related forms, avoiding the Set as well.
The case of SocketException("Connection reset") was not handled,
and IOException only checked for one message. This combines the
logic for errnoFromException with the logic for errnorFromMessage.
The exception handling stack was also cleaned up here.
Member
Author
|
Risks from this PR:
Other notes:
|
enebo
reviewed
Oct 18, 2023
This code previously assumed that all -1 return values from read meant EAGAIN and that we should proceed to selection. If the read result was actually equivalent to EAGAIN, this was correct. If the read result was some other error, we still would proceed to the selection, and in most cases the same or similar error would be raised at that point. However at least one particular case: a "Connection reset" event caused by the other end sending RST (due in this case to SO_LINGER=0), the selection operation did *not* raise any error, and appeared to be readable. As a result we went back to the read, another "Connection reset" was triggered, we assumed it was EAGAIN, and so on. This commit checks the errno for exactly matching EAGAIN or EWOULDBLOCK before performing the selection, otherwise raising an error for the failed read. Fixes jruby#7961
headius
force-pushed
the
socket_rst_fixes
branch
from
d03aecf to
51464f6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JRuby IO subsystem simulates a POSIX environment by capturing Java exceptions and translating them to their equivalent errno values. When a read produces an error, we return -1 and set the local shim's errno. This can then be used by POSIX-like consumers of the API to process the error without exception-handling.
However in the case of IO read operations, we were not appropriately checking the errno result for a failed (i.e. -1) read. Instead we assumed it was always an EAGAIN-like result and proceeded to select for read on the socket. Under normal circumstances, like the socket being fully closed on both ends, that select would raise an error anyway so no errors were lost.
In the example from #7961 (and likely the case in #6346), only one side of the socket was closed using SO_LINGER=0, causing an RST packet to be returned to future reads, manifested by the
readoperation as a SocketException("Connection reset"). The logic described above then proceeded directly to aselectoperation, which did not in this case raise any error. Instead,selectreported that the socket was still readable, so we loop back to thereadad infinatum.The changes here make three changes:
readreturns -1, only proceed to selection if the errno result wasEAGAIN/EWOULDBLOCK, otherwise raising the appropriate error.selectresults.Fixes #7961 and #6346.