preventing hang when TCPSocket readable, but no app-data is available#232
preventing hang when TCPSocket readable, but no app-data is available#232dotconfig404 wants to merge 1 commit into
Conversation
|
fixes #233 |
dotconfig404
force-pushed
the
patch-1
branch
2 times, most recently
from
9aa8dce to
9e82856
Compare
| @socket.close | ||
| connect | ||
| elsif @socket.io.to_io.wait_readable(0) && @socket.eof? | ||
| elsif @socket.io.to_io.wait_readable(0) && @socket.io.read_nonblock(1, exception: false).nil? |
There was a problem hiding this comment.
The direction of this fix is correct. But this fix has a problem.
If this read_nonblock doesn't return a byte, it works as expected. But if it returns a byte, it needs to go forward but the byte is dropped in this patch. The byte needs to be buffered and re-used in actual process
There was a problem hiding this comment.
Thanks for the review. I see your point, although it shouldn't ever happen that a byte is returned, this is not the best way.
There was a problem hiding this comment.
hi again, does the new version look better?
It pushes back the byte should there really be a byte returned using ungetc to @rbuffer in OpenSSL::Buffering
dotconfig404
force-pushed
the
patch-1
branch
2 times, most recently
from
743344b to
53d145a
Compare
|
i force-pushed a new version. it alters the way the existing code reviews look like now, so maybe that was wrong. hope it's ok, otherwise ill be happy to close and create a new PR. would like some quick feedback on what is preferred in this case. |
rhenium
left a comment
rhenium
left a comment
There was a problem hiding this comment.
ungetc seems like a much simpler solution.
I think the EOFError branch can be removed, but otherwise this looks good to me.
| rescue EOFError | ||
| true |
There was a problem hiding this comment.
I suspect this EOFError branch is unreachable.
3 participants
eof? is a blocking read that times out when no application data is available - it should not be used as a readiness probe