GH-118095: Make invalidating and clearing executors memory safe - #118459
GH-118095: Make invalidating and clearing executors memory safe#118459markshannon merged 4 commits into
Conversation
Sorry, something went wrong.
|
There are no new tests as I can't see a way to test this, especially the out-of-memory checks. |
Sorry, something went wrong.
| const _PyExecutorObject *side = executor->exits[i].executor; | ||
| executor->exits[i].temperature = initial_unreachable_backoff_counter(); | ||
| if (side != cold) { | ||
| executor->exits[i].executor = cold; |
There was a problem hiding this comment.
Looks like the executor owns the reference to exits[i].executor when that is not equal to the cold exit, but not when it is equal. That is potentially confusing.
Sorry, something went wrong.
There was a problem hiding this comment.
It owns a reference in both cases, it is just that cold exit executors are immortal, so we can be lose with the refcounting.
Sorry, something went wrong.
| _Py_ExecutorClear(exec); | ||
| unlink_executor(exec); | ||
| if (no_memory) { | ||
| exec->vm_data.valid = 0; |
There was a problem hiding this comment.
It's really ok to just skip clearing exec in this case?
Sorry, something went wrong.
There was a problem hiding this comment.
We never execute an executor with valid == 0, so it does what says and invalidates the dependent executor.
It might delay later re-optimizations and defer reclaiming some objects.
We've hit a memory error at this point, so the program is probably dying anyway.
Sorry, something went wrong.
There was a problem hiding this comment.
We've hit a memory error at this point, so the program is probably dying anyway.
That feels like a dangerous assumption -- an app may have a gigabyte array of temp data somewhere that it can free and retry.
Sorry, something went wrong.
|
The 32 bit Windows Jit build failure is probably unrelated as it has been flaky lately. |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot ARM64 MacOS M1 Refleaks NoGIL 3.x has failed when building commit f6fab21. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1368/builds/882 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (most recent call last):
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/test/test_concurrent_futures/executor.py", line 131, in test_free_reference
self.assertIsNone(wr())
~~~~~~~~~~~~~~~~~^^^^^^
AssertionError: <test.test_concurrent_futures.executor.MyObject object at 0x20006070090> is not None
Traceback (most recent call last):
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/test/test_threading.py", line 2035, in test_interrupt_main_subthread
t.start()
~~~~~~~^^
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 977, in start
self._started.wait() # Will set ident and native_id
~~~~~~~~~~~~~~~~~~^^
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 656, in wait
with self._cond:
...<3 lines>...
return signaled
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 307, in __exit__
return self._lock.__exit__(*args)
~~~~~~~~~~~~~~~~~~~^^^^^^^
RuntimeError: release unlocked lock
Traceback (most recent call last):
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 659, in wait
signaled = self._cond.wait(timeout)
~~~~~~~~~~~~~~~^^^^^^^^^
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 368, in wait
self._acquire_restore(saved_state)
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/threading.py", line 315, in _acquire_restore
def _acquire_restore(self, x):
KeyboardInterrupt
|
Sorry, something went wrong.
| exec->vm_data.valid = 0; | ||
| } | ||
| } | ||
| if (is_invalidation) { |
There was a problem hiding this comment.
I don't see any calls where this flag is passed as 0, so maybe the flag argument is no longer needed?
Sorry, something went wrong.
This fixes a crash when an executor forms a reference cycle with itself.
This bug has been present for a while, but was only exposed by #118420.
Also fixes another latent bug in _Py_Executors_InvalidateDependency, where clearing one executor could clear another leaving the iteration variable pointing to freed memory.
The three main changes amongst the main changes are: