gh-108240: Add _PyCapsule_SetTraverse() internal function by vstinner · Pull Request #108339 · python/cpython · GitHub

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.

Issue: _socket extension C capsule cannot be visited by the GC and so _socket.socket type may stay alive longer than expected #108240

📚 Documentation preview 📚: https://cpython-previews--108339.org.readthedocs.build/

@erlend-aasland: Here is a more complete solution to add traverse/clear functions to the socket C API capsule.

IMO, I like this solution more than #108241, because #108241 looks like a solution of corner case.

Maybe this function should not be added to the limited C API.

test_ssl fails on macOS and altered the environment when re-run in verbose mode:

ERROR: test_preauth_data_to_tls_client (test.test_ssl.TestPreHandshakeClose.test_preauth_data_to_tls_client)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/test_ssl.py", line 4802, in test_preauth_data_to_tls_client
    client.connect(server.listener.getsockname())
ConnectionResetError: [Errno 54] Connection reset by peer
(...)
Warning -- threading_cleanup() failed to cleanup 0 threads (count: 0, dangling: 2)
Warning -- Dangling thread: <SingleConnectionTestServerThread(preauth_data_to_tls_client, stopped 123145440923648)>
Warning -- Dangling thread: <_MainThread(MainThread, started 4610459136)>

test.pythoninfo:

ssl.OPENSSL_VERSION: OpenSSL 3.0.10 1 Aug 2023
ssl.OPENSSL_VERSION_INFO: (3, 0, 0, 10, 0)

@erlend-aasland: Here is a more complete solution to add traverse/clear functions to the socket C API capsule.

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?

I removed the new function from the limited C API.

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.

Since this function is only used for a single extension yet, the _socket extension, I changed my PR to only add the function to the internal C API (Py_BUILD_CORE). This function can be made public later if needed.

@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 sys.modules.

You could have used the internal APIs here:

Suggested change

      
                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.

I used _PyObject_GC_IS_TRACKED() in my second PR, but without assert(_PyObject_IS_GC(op)) which looks overkill.

else is unneeded here.

Currently, the callback can be NULL. I added a check in my second PR to reject NULL callbacks.

@Eclips4 @erlend-aasland: I chose the middle ground, start by making the API internal.

I agree that keeping it internal makes sense for now. I do think a public API like this is needed, though.

It's not worth it to backport the change it's a corner case to manually unload a module from sys.modules.

I agree that we should not backport this PR.

@erlend-aasland: I wrote PR #108417 to address your review.

vstinner requested review from a team and encukou as code owners August 22, 2023 20:17

bedevere-bot mentioned this pull request Aug 22, 2023

_socket extension C capsule cannot be visited by the GC and so _socket.socket type may stay alive longer than expected #108240

Closed

2 tasks

bedevere-bot added the awaiting core review label Aug 22, 2023

vstinner force-pushed the capsule_traverse branch from bf9c9eb to d661ec5 Compare August 22, 2023 20:17

vstinner commented Aug 22, 2023

View reviewed changes

vstinner force-pushed the capsule_traverse branch from d661ec5 to e76f712 Compare August 23, 2023 15:16

vstinner force-pushed the capsule_traverse branch from e76f712 to 1cf5423 Compare August 23, 2023 21:40

vstinner changed the title gh-108240: Add PyCapsule_SetTraverse() function gh-108240: Add _PyCapsule_SetTraverse() internal function Aug 23, 2023

pythongh-108240 : Add _PyCapsule_SetTraverse() interal function

70517e9

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.

vstinner added the skip news label Aug 23, 2023

vstinner force-pushed the capsule_traverse branch from 1cf5423 to 70517e9 Compare August 23, 2023 21:41

vstinner merged commit 513c89d into python:main Aug 23, 2023

bedevere-bot removed the awaiting core review label Aug 23, 2023

vstinner deleted the capsule_traverse branch August 23, 2023 22:19

vstinner mentioned this pull request Aug 23, 2023

gh-108240: Fix a reference cycle in _socket.CAPI capsule #108241

Closed

erlend-aasland reviewed Aug 24, 2023

View reviewed changes

ZeroIntensity mentioned this pull request Sep 25, 2024

v3.13.0rc2 Segmentation Fault in gc.get_referents. PyCapsule->traverse_func is NULL #124538

Closed

+                  else {
+                      return 0;
+                  }

gh-108240: Add _PyCapsule_SetTraverse() internal function by vstinner · Pull Request #108339 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-108240: Add _PyCapsule_SetTraverse() internal function - #108339

gh-108240: Add _PyCapsule_SetTraverse() internal function#108339vstinner merged 1 commit intopython:mainpython/cpython:mainfrom vstinner:capsule_traversevstinner/cpython:capsule_traverseCopy head branch name to clipboard

Conversation

vstinner commented Aug 22, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Aug 22, 2023

Uh oh!

Eclips4 commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Aug 22, 2023

Uh oh!

erlend-aasland commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

erlend-aasland Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Aug 24, 2023

Uh oh!

vstinner commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-108240: Add _PyCapsule_SetTraverse() internal function#108339
vstinner merged 1 commit into
python:mainfrom
vstinner:capsule_traverse

vstinner commented Aug 22, 2023 •
edited by github-actions Bot
Loading

Eclips4 commented Aug 22, 2023 •
edited
Loading