-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-140795: Remove 'exc' field in SSLObject #143491
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
The 'exc' field was used by our debug SSL callbacks. Keep the exception in the normal per-thread state to avoid shared mutable state between threads. This also avoids a reference count leak if the Python callback raised an exception because it can be called multiple times per SSL operation.
|
This is like: But for the |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 8cd4d98 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143491%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
vstinner
left a comment
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.
LGTM, but I would feel safer with proposed assertions to check that assumptions remain valid in the future if the code is modified.
| } | ||
| Py_RETURN_NONE; | ||
| error: | ||
| Py_XDECREF(sock); |
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.
You might add: assert(exc == NULL);.
| } | ||
| return PyLong_FromSize_t(count); | ||
| error: | ||
| Py_XDECREF(sock); |
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.
You might add: assert(exc == NULL);.
| int retval; | ||
| int sockstate; | ||
| _PySSLError err; | ||
| PyObject *exc; |
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.
For consistency with other functions:
| PyObject *exc; | |
| PyObject *exc = NULL; |
| return PyLong_FromSize_t(count); | ||
| } | ||
|
|
||
| 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.
You might add: assert(exc == NULL); here and in the done: label.
| Py_RETURN_NONE; | ||
|
|
||
| error: | ||
| Py_XDECREF(sock); |
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.
You might add: assert(exc == NULL);.
|
The 4 failed builbots are unrelated to this change. Windows buildbots failed to download zlib external dependency:
Android buildbot: test_urllibnet failed with: |
The 'exc' field was used by our debug SSL callbacks. Keep the exception in the normal per-thread state to avoid shared mutable state between threads.
This also avoids a reference count leak if the Python callback raised an exception because it can be called multiple times per SSL operation.