GeezerVison

This change basically reverts commit 01157e0 (PR gh-139449) which fixed another deadlock on tracemalloc.stop() :-) See issue gh-139116.

cc @ZeroIntensity @encukou

This change basically reverts commit 01157e0 (PR #139449) which fixed another deadlock on tracemalloc.stop() :-) See issue #139116.

I pushed a change to avoid the deadlock on tracemalloc.stop(): don't call PyRefTracer_SetTracer() while holding TABLES_LOCK().

Is it necessary to keep the attached thread state in TABLES_LOCK? It seems the bulk of the fix is just calling PyRefTracer_SetTracer without holding the lock.

There are two problems:

• (A) detaching the thread state in TABLES_LOCK() breaks a lot of assumptions in the _io module about critical sections, the GC and memory allocators leading to race conditions. See Crash in _io_TextIOWrapper_write_impl when running tracemalloc.clear_traces in threads #144763 (comment) for the problem on the _io.TextIOWrapper.write() method.
• (B) Deadlock involving PyRefTracer_SetTracer() in tracemalloc.stop(): see issue Potential deadlock in test_tracemalloc with the FT build and tail-call dispatch. #139116. My fix is to call PyRefTracer_SetTracer() without holding TABLES_LOCK() (which is safe at this point in _PyTraceMalloc_Stop()).

Yes, I think that it's necessary to keep the attached thread state in TABLES_LOCK (not detach the thread state).

Alternatively, we could just use Py_BEGIN_CRITICAL_SECTION_MUTEX instead of PyMutex_Lock.

What would be the benefits?

Using _Py_LOCK_DONT_DETACH in TABLES_LOCK() definitely seems like the right approach for the allocator calls (i.e., tracemalloc_alloc/tracemalloc_free). We don't want to detach the thread state there.

I'm not really sure about _PyTraceMalloc_Stop() and _PyTraceMalloc_Start(). Should these be safe to call concurrently? The implementation isn't thread-safe for concurrent calls to tracemalloc.start() or tracemalloc.stop(). Maybe that's fine?

I'm not really sure about _PyTraceMalloc_Stop() and _PyTraceMalloc_Start(). Should these be safe to call concurrently? The implementation isn't thread-safe for concurrent calls to tracemalloc.start() or tracemalloc.stop(). Maybe that's fine?

I'm only aware of #128679 : concurrent calls to PyTraceMalloc_Track() and tracemalloc.stop() (case tested by test_tracemalloc.test_tracemalloc_track_race()).

I don't think that two threads should be able to call tracemalloc.stop() and tracemalloc.start() in parallel. I expect that only a single thread (the main thread?) would call these two methods.

Could we use _Py_LOCK_DONT_DETACH in TABLES_LOCK, but use a normal (detaching) call in start/stop? That should avoid the reftracer deadlock without needing to move the call to PyRefTracer_SetTracer.

Nevermind, that would lead to a lock-ordering deadlock:

1. Thread A releases the GIL and acquires TABLES_LOCK.
2. Thread B acquires the GIL and begins waiting on TABLES_LOCK.
3. Thread A is waiting on the GIL, which thread B holds.
4. Deadlock!

If I remember correctly, tracemalloc still needs more work to avoid crashes when running concurrently (particularly in subinterpreters), so I think it would be fine to leave start/stop as is in this PR and address it later.

@kumaraditya303 @colesbury @ZeroIntensity: So what do you think of this change? Does it look good to you?

GH-144965 is a backport of this pull request to the 3.14 branch.

Backport of the fix to 3.14: #144965. The backport is different since the 3.14 code already avoided to detach the thread safe.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD Refleaks 3.14 (tier-3) has failed when building commit f67cf83.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1800/builds/1004) and take a look at the build logs.
4. Check if the failure is related to this commit (f67cf83) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1800/builds/1004

Test leaking resources:

• test_events: references
• test_events: memory blocks

Summary of the results of the build (if available):

==

Traceback (most recent call last):
  File "/buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/test/support/__init__.py", line 859, in gc_collect
    gc.collect()
ResourceWarning: unclosed <socket.socket fd=9, family=2, type=1, proto=6, laddr=('127.0.0.1', 15216), raddr=('127.0.0.1', 15217)>
Task was destroyed but it is pending!
task: <Task pending name='Task-4077' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/asyncio/selector_events.py:217> wait_for=<Future finished exception=ConnectionResetError()>>
Future exception was never retrieved
future: <Future finished exception=ConnectionResetError()>
Traceback (most recent call last):
  File "/buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/asyncio/sslproto.py", line 581, in _on_handshake_complete
    raise handshake_exc
ConnectionResetError
Warning -- Unraisable exception
Exception ignored while calling deallocator <function _SelectorTransport.__del__ at 0x847dcc4d0>:
Traceback (most recent call last):
  File "/buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/asyncio/selector_events.py", line 873, in __del__
    _warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
ResourceWarning: unclosed transport <_SelectorSocketTransport closing fd=9>
k


Traceback (most recent call last):
  File "/buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/test/support/__init__.py", line 859, in gc_collect
    gc.collect()
ResourceWarning: unclosed <socket.socket fd=8, family=2, type=1, proto=6, laddr=('127.0.0.1', 11443), raddr=('127.0.0.1', 11444)>
Task was destroyed but it is pending!
task: <Task pending name='Task-4078' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/asyncio/selector_events.py:217> wait_for=<Future pending cb=[Task.task_wakeup()]>>
Warning -- Unraisable exception
Exception ignored while calling deallocator <function _SelectorTransport.__del__ at 0x844ba84d0>:
Traceback (most recent call last):
  File "/buildbot/buildarea/3.14.ware-freebsd.refleak/build/Lib/asyncio/selector_events.py", line 873, in __del__
    _warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
ResourceWarning: unclosed transport <_SelectorSocketTransport closing fd=8>
k