-
-
Notifications
You must be signed in to change notification settings - Fork 99
Fixes for WantWriteError and WantReadError handling #764
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
base: main
Are you sure you want to change the base?
Conversation
90992b8
to
f7fc67b
Compare
f7fc67b
to
16ecc66
Compare
I clicked "rebase" since |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
@julianz- do you think your thing will let us make use of pyca/pyopenssl#954 ? |
16ecc66
to
f39e8f0
Compare
I made a comment about the code there but did you want me to add in your change to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move unrelated typing updates into a separate PR? Ideally, we should just migrate the annotations from stubs into Python modules. But that'd be another effort. I think pyrefly autotype
might help — I've tried it on a portion of a different code base and it was able to inject some types (that required post-processing, though).
That's a long-standing idea. And I've lost some context over the years. But I think I wanted us to use |
5c4225d
to
828b1b1
Compare
I think the change there is pretty similar so it might be possible though maybe we should just get this PR done first? |
828b1b1
to
8ac02bf
Compare
Yes, let's do so. |
3554f0a
to
67137c6
Compare
Some tests fail because the timeouts on SSL tests are too short. def http_request_timeout(): Also saw another timeout failure on test_keepalive_conn_management() |
67137c6
to
34970d8
Compare
fd4d129
to
bbfcc7e
Compare
@julianz- thanks for that PR, I've posted some comments. But could you be more specific when you're talking about the failures your saw — linking specific failing job logs would be helpful for me to understand the context better. |
2442063
to
c9379be
Compare
Added handling for WantWriteError and WantReadError in BufferedWriter and StreamReader to enable retries. This addresses long standing issues discussed in cherrypy#245. The reliability of the fix relies on using pyOpenSSL v25.2.0 or greater, as earlier versions have known bugs that affect the retry logic.
c9379be
to
4e4afa4
Compare
@hardikmodha it needs some work + more review. We're working on a few other PRs that improve a few more generic things. I imagine Julian and I will get back to this once those PRs are in. |
import socket | ||
import time | ||
|
||
from OpenSSL import SSL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this will have to become conditional/guarded since pyOpenSSL is an optional dependency. It might be a good idea to add infra for running the tests w/o optional deps but that certainly out of the scope of this PR.
# This catches errors like EBADF (Bad File Descriptor) | ||
# or EPIPE (Broken pipe), which indicate the underlying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be made more specific instead of a broad OSError
.
): | ||
# these errors require retries with the same data | ||
# regardless of whether data has already been written | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these leak into the generic writer? Is there an underlying layer where we could catch these and re-raise and common/generic exceptions? Can we scope accessing PyOpenSSL to the pyopenssl.py
module?
If not, we'll probably have to have a common var in errors.py
but I don't really like leaking this into outer layers. Need to think of a better structure while we're on it.
for _ in range(MAX_ATTEMPTS): | ||
try: | ||
val = super().read(*args, **kwargs) | ||
except (SSL.WantReadError, SSL.WantWriteError) as ssl_want_error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've basically the same concern with leaking PyOpenSSL into the outer scope here as in https://github.com/cherrypy/cheroot/pull/764/files#r2432889204.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed.
@@ -0,0 +1,7 @@ | |||
Added handling for WantWriteError and WantReadError in BufferedWriter | |||
and StreamReader to enable retries. This addresses long standing issues | |||
discussed in #245. The reliability of the fix relies on using pyOpenSSL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use :issue:`245`
for refs. It's not Markdown.
Additionally, you could symlink this change note to that number as well so both will be linked in the change log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symlink 245.bugfix.rst
to this file.
Added handling for WantWriteError and WantReadError in BufferedWriter | ||
and StreamReader to enable retries. This addresses long standing issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these are linkable in Sphinx. pyOpenSSL is plugged via intersphinx. And internal objects are exposed too.
See these to discover the refs:
This adds improved handling for WantWriteError and WantReadError so that it retries writing the full buffer in BufferedWriter and retries reading in BufferedReader after a short delay allowing for network buffers to settle. The retry logic also makes use of new changes in PyOpenSSL v25.2.0 and Cryptography v45.0.7 that allow for a moving buffer as discussed in #245.
This change is