Uh oh!
There was an error while loading. Please reload this page.
gh-108240: Add _PyCapsule_SetTraverse() internal function - #108339
Conversation
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. |
vstinner
left a comment
There was a problem hiding this comment.
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.
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.
I used _PyObject_GC_IS_TRACKED() in my second PR, but without assert(_PyObject_IS_GC(op)) which looks overkill.
| else { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
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/