Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-108240: Add _PyCapsule_SetTraverse() internal function#108339
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Aug 22, 2023 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
bf9c9eb to d661ec5Comparevstinner commented Aug 22, 2023
@erlend-aasland: Here is a more complete solution to add traverse/clear functions to the socket C API capsule. |
Eclips4 commented Aug 22, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Maybe this function should not be added to the limited C API.
vstinner commented Aug 22, 2023
test_ssl fails on macOS and altered the environment when re-run in verbose mode: test.pythoninfo: |
erlend-aasland commented Aug 23, 2023
As I mentioned to you earlier, I had thoughts of a similar API, and I think this is a better solution to the problem than #108241. I think we should get more eyes on the API, though. If we are to make it public right away, we should be sure to get it right the first time ;) Perhaps create a topic on Discourse? |
d661ec5 to e76f712Comparevstinner commented Aug 23, 2023
I removed the new function from the limited C API. |
e76f712 to 1cf5423CompareThe _socket extension uses _PyCapsule_SetTraverse() to visit and clear the socket type in the garbage collector. So the _socket.socket type can be cleared in some corner cases when it wasn't possible before.
1cf5423 to 70517e9Comparevstinner commented Aug 23, 2023
Since this function is only used for a single extension yet, the |
vstinner commented Aug 23, 2023
@Eclips4@erlend-aasland: I chose the middle ground, start by making the API internal. I tested manually that the change fix issue #108240 leak. It's not worth it to backport the change it's a corner case to manually unload a module from |
| if (!PyObject_GC_IsTracked(op)){ | ||
| PyObject_GC_Track(op); |
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 could have used the internal APIs here:
| if (!PyObject_GC_IsTracked(op)){ | |
| PyObject_GC_Track(op); | |
| assert(_PyObject_IS_GC(op)); | |
| if (!_PyObject_GC_IS_TRACKED(op)){ | |
| _PyObject_GC_TRACK(op); | |
| } |
OTOH, _PyCapsule_SetTraverse is probably not part of hot code, so I guess the public APIs are fine.
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 used _PyObject_GC_IS_TRACKED() in my second PR, but without assert(_PyObject_IS_GC(op)) which looks overkill.
| else{ | ||
| return0; | ||
| } |
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.
else is unneeded here.
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.
Currently, the callback can be NULL. I added a check in my second PR to reject NULL callbacks.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland commented Aug 24, 2023
I agree that keeping it internal makes sense for now. I do think a public API like this is needed, though.
I agree that we should not backport this PR. |
vstinner commented Aug 24, 2023
@erlend-aasland: I wrote PR #108417 to address your review. |
The _socket extension uses PyCapsule_SetTraverse() to visit and clear the socket type in the garbage collector. So the _socket.socket type can be cleared in some corner cases when it wasn't possible before.
📚 Documentation preview 📚: https://cpython-previews--108339.org.readthedocs.build/