GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-109978: Allow multiprocessing finalizers to run on a separate thread by pitrou · Pull Request #110510 · python/cpython

Conversation

pitrou commented Oct 7, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Allow multiprocessing finalizers to run on a separate thread #109978

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot mentioned this pull request

Oct 7, 2023

Allow multiprocessing finalizers to run on a separate thread #109978

Open


          pythonGH-109978: Allow multiprocessing finalizers to run on a separat…

3664a79

…e thread

pitrou force-pushed the gh109978-mp-finalizer-thread branch from a7d3581 to 3664a79 Compare

October 7, 2023 21:25

pitrou commented Oct 7, 2023

Copy link

Copy Markdown

Member Author

This is an attempt at solving the aforementioned issue (let non-reentrant multiprocessing finalizers run on a separate thread)... but there are complications due to the fact that multiprocessing obviously relies on fork, which strongly prefers a single-thread setup.

@vstinner @gpshead

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou commented

Oct 7, 2023

View reviewed changes

Comment thread

Lib/multiprocessing/util.py

Comment on lines +225 to +230

		if thread_was_running:
		# HACK: os.fork() queries the system for the number of running threads.
		# However, Thread.join() only ensures that the Python thread state
		# was destroyed, while the system thread could still be running.
		# Give it time to exit.
		time.sleep(0.001)

pitrou Oct 7, 2023

Copy link

Copy Markdown

Member Author

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.

This is a bit unfortunate. @gpshead

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead Oct 13, 2023

Copy link

Copy Markdown

Member

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.

Agreed, this is effectively because our join() has never truely been a join... I filed #110829 to track that, it is probably time to fix that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner commented Oct 12, 2023

Copy link

Copy Markdown

Member

I think that it will skip my turn for this one. I touched too many multiprocessing code recently, and I was bitten.

Maybe @serhiy-storchaka wants to have a look, he did something like that recently (I recall vaguely).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka self-requested a review

October 13, 2023 12:48

gpshead mentioned this pull request

Oct 13, 2023

threading Thread.join should call the OS join API #110829

Closed

gpshead reviewed

Oct 13, 2023

View reviewed changes

gpshead left a comment

Copy link

Copy Markdown

Member

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 haven't tried to understand the whole thing yet, but i've given it a once over to look at the issues this is running into.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py

Comment on lines +225 to +230

		if thread_was_running:
		# HACK: os.fork() queries the system for the number of running threads.
		# However, Thread.join() only ensures that the Python thread state
		# was destroyed, while the system thread could still be running.
		# Give it time to exit.
		time.sleep(0.001)

gpshead Oct 13, 2023

Copy link

Copy Markdown

Member

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.

Agreed, this is effectively because our join() has never truely been a join... I filed #110829 to track that, it is probably time to fix that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/posixmodule.c

		}
		// XXX This counts the number of system threads, but Python code can only
		// call threading.Thread.join(), which can return before the system thread ended.
		// This function could therefore print a spurious warning in unlucky cases.

gpshead Oct 13, 2023

Copy link

Copy Markdown

Member

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.

It's not spurious though, the existence of a thread doing anything is accurately identified as a potential problem. The annoyance that led to this though is that Python didn't provide a concrete way to guarantee a thread has exited.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka reviewed

Oct 13, 2023

View reviewed changes

Comment thread

Lib/multiprocessing/pool.py

Comment on lines +669 to +670

		while self._pool:
		self._pool.pop().join()

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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.

What is the difference?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py

Comment on lines +242 to +243

		with self._lock():
		return self._stopped

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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.

Why is the lock needed for reading an attribute?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py


		def wait_until_idle(self):
		with self._queue_drained_cond:
		self._queue_drained_cond.wait_for(lambda: self._queue.empty())

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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.

Suggested change

	self._queue_drained_cond.wait_for(lambda: self._queue.empty())
	self._queue_drained_cond.wait_for(self._queue.empty)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py

		# from the work loop.
		assert callable(cb)
		with self._lock:
		if not self._stopped and self._thread is not None:

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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.

Isn't self._thread always None when self._stopped is True?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py

Comment on lines +288 to +289

		finally:
		cb = None

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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 add a reference to _work_loop() or copy the comment? I was confused when I saw this.

Hmm, _work_queue.enqueue_task() is always called from the global enqueue_task()which keeps a reference tocb`, so it perhaps does not help.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/util.py

		sub_debug('finalizer ignored because different process')
		res = None
		else:
		try:

serhiy-storchaka Oct 13, 2023

Copy link

Copy Markdown

Member

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.

There is already one try a level above. The second try may be not needed, you can simply add finally at the same level after else. The difference is that the finally block will be exececuted in the except case, but I do not see what can be wrong with this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou commented Oct 15, 2023

Copy link

Copy Markdown

Member Author

FWIW, I'm prioritizing #110848 now, as it should make this PR slightly more robust.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Apr 14, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 14, 2026

ambv commented Oct 21, 2026

Copy link

Copy Markdown

Contributor

This PR is blocked on its author, has not been touched by a human in more than 3 years, and no longer merges cleanly. Closing to cull the number of open pull requests. Do not reopen this pull request without getting a go-ahead from a triager or core developer on gh-109978.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ambv closed this

Oct 21, 2026

Reviewers

gpshead

gpshead left review comments

serhiy-storchaka

serhiy-storchaka left review comments

vsajip

Awaiting requested review from vsajip

vsajip will be requested when the pull request is marked ready for review

vsajip is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

stale

Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

pitrou commented Oct 7, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Oct 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 12, 2023

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 15, 2023

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

ambv commented Oct 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou commented Oct 7, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.