Fix race conditions and improve robustness during socket I/O #779
Conversation
julianz-
changed the title
julianz-
force-pushed
the
fix-socket-teardown
branch
2 times, most recently
from
887399d to
4f1662e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
+ Coverage 79.30% 79.33% +0.03%
==========================================
Files 29 29
Lines 4203 4269 +66
Branches 539 543 +4
==========================================
+ Hits 3333 3387 +54
- Misses 728 735 +7
- Partials 142 147 +5 |
julianz-
force-pushed
the
fix-socket-teardown
branch
from
4f1662e to
4833dac
Compare
julianz-
changed the title
julianz-
changed the title
julianz-
force-pushed
the
fix-socket-teardown
branch
2 times, most recently
from
048d898 to
f0471ca
Compare
|
@julianz- could you scan the modules more broadly for places where we could stop suppressing connection errors on the low level layers? I'm not looking into the tests until we figure out the architectural overview of the whole thing. But I feel like we're getting closer. I think we might need to split this PR into two at some point, will see. |
|
Thank you @webknjaz for all your great suggestions and points. I will take a look at everything. The layering is actually more complicated than I had realized. |
julianz-
force-pushed
the
fix-socket-teardown
branch
3 times, most recently
from
b26544f to
f7d8469
Compare
Yep, that's why I have incomplete pieces of code somewhere locally that I never finalized. It does require some effort.. |
julianz-
force-pushed
the
fix-socket-teardown
branch
from
f7d8469 to
b0587dd
Compare
There was a problem hiding this comment.
I think the current state of this module is good but Codecov shows that none of the newly added lines are covered with tests. We'll have to fix this before merging.
It looks like the updates to cheroot.ssl.pyopenssl could go into a standalone pull request. Could you extract this and add tests+changenote in a separate PR? I hope this will simplify reviewing this one.
| errno.EPIPE, | ||
| errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3 | ||
| errno.ECONNRESET, # corresponds to ConnectionResetError in Python 3 | ||
| *((errno.WSAENOTSOCK,) if _compat.IS_WINDOWS else ()), |
There was a problem hiding this comment.
Does this exception correspond to EBADF semantically? If so, I'd probably keep them close to each other as suggested in https://github.com/cherrypy/cheroot/pull/779/files#r2432797948.
Additionally, could you add a comment hinting what might be causing it in practice?
| 'sendall', | ||
| 'settimeout', | ||
| 'gettimeout', | ||
| 'shutdown', |
There was a problem hiding this comment.
Will this interferre with the newly added decorator/methods? proxy_wrapper() below does effectively what we did with @_morph_syscall_to_connection_error and so shutdown() will end up double-decorated/locked if I'm reading this correctly.
| 'accept', | ||
| 'setblocking', | ||
| 'fileno', | ||
| 'close', |
There was a problem hiding this comment.
Will this interferre with the newly added decorator/methods? proxy_wrapper() below does effectively what we did with @_morph_syscall_to_connection_error and so shutdown() will end up double-decorated/locked if I'm reading this correctly.
julianz-
force-pushed
the
fix-socket-teardown
branch
from
b0587dd to
97cad94
Compare
Fixes to make socket I/O more resilient during connection teardown. 1. BufferedWriter's write(): Added error handling to ignore common socket errors (e.g., ECONNRESET, EPIPE, ENOTCONN, EBADF) that occur when the underlying connection has been unexpectedly closed by the client or OS. This prevents a crash when attempting to write to a defunct socket. 2. BufferedWriters's close(): Made idempotent, allowing safe repeated calls without raising exceptions. 3. Needed to add explicit handling of WINDOWS environments as these are seen to throw Windows specific WSAENOTSOCK errors. Includes new unit tests to cover the idempotency and graceful handling of already closed underlying buffers.
julianz-
force-pushed
the
fix-socket-teardown
branch
from
97cad94 to
342ecf6
Compare
2 participants
Fixed race conditions to make socket I/O more resilient during connection teardown.
write(): Added error handling to ignore common socket errors (e.g.,ECONNRESET,EPIPE,ENOTCONN,EBADF) that occur when the underlying connection has been unexpectedly closed by the client or OS. This prevents a crash when attempting to write to a defunct socket.close(): Made idempotent, allowing safe repeated calls without raising exceptions.WSAENOTSOCKerrors.Includes new unit tests to cover the idempotency and graceful handling of already closed underlying buffers.
This change is