gh-91090: Make started multiprocessing.Process instances and started multiprocessing.managers.BaseManager instances serialisable

[geryogam]

Since starting a multiprocessing.Process instance p stores a weak reference at p._popen.finalizer._weakref which makes p unserialisable, we solve that issue by storing only the _key of the finalizer (instead of the whole finalizer) whose mapping to the finalizer is stored in the multiprocessing.util._finalizer_registry dictionary.

Likewise, we also make started multiprocessing.managers.BaseManager instances serialisable.

• Issue: Started multiprocessing.Process instances are unserialisable #91090

[saurabh02]

Up-voting this PR and hoping it would be merged soon!

[kumaraditya303]

I'll take a look at this soon but it needs tests and news entry.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[erlend-aasland]

@maggyero, please avoid force-pushing; instead use git merge --no-ff main to sync with main. Please take a look at the Life-Cycle of a Pull Request chapter in the devguide: https://devguide.python.org/getting-started/pull-request-lifecycle/

[geryogam]

@erlend-aasland Thanks for the review requests and force-pushing advice. About the latter, I actually did not rebase my branch on the main branch and force pushed (‘Update with rebase’ in the GitHub UI), I just amended my last commits and force pushed. The link you provided says the following about force pushing:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

Your pull request may involve several commits as a result of addressing code review comments. Please keep the commit history in the pull request intact by not squashing, amending, or anything that would require a force push to GitHub. A detailed commit history allows reviewers to view the diff of one commit to another so they can easily verify whether their comments have been addressed. The commits will be squashed when the pull request is merged.

I am wondering if this is a valid exception to this rule: is it okay for a developer to amend and force push the commits that were pushed after the last code review comment, for instance if he realizes that he made a mistake or forgot something while addressing the reviewers’ comments? Basically like I did: I did not touch the commits preceding @kumaraditya303’s comment but amended some following commits and force pushed. Or is it a strict rule (never force push)?

[geryogam]

I'll take a look at this soon but it needs tests and news entry.

@kumaraditya303 I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@kumaraditya303: please review the changes made to this pull request.

[erlend-aasland]

Regarding force-pushing:

• even though nobody has left (more recent) comments, a review may be in progress (remember: a review may take days to complete); if the commits are rewritten because of amendments, this will mess up a pending review
• CI runs are tied to commits; losing the old commits will make it hard to find and investigate previous CI runs
• as the devguide says, merging to main is done by squashing the commits; rewriting a commit message is not a valid reason to force-push

Eric left a comment regarding force-pushing on a PR a while ago; it is worth a read: #31616 (comment)

I'll put some more emphasis on being respectful of the reviewer's time: IMO, one of the best things one can do in order to respect the reviewers time is to keep PRs focused on the task in question, and that only. Mixing an enhancement or a bugfix with unrelated stylistic changes, adding multiple features in one PR, etc., makes it hard, if not to say impossible, to review a PR.

[geryogam]

Those are convincing arguments, thanks for making me aware of this. I suppose that the only case where a force push could be tolerated is when the PR is in draft state. But once it is published, it is over.

[erlend-aasland]

I suppose that the only case where a force push could be tolerated is when the PR is in draft state. But once it is published, it is over.

Yes, that's correct. A draft PR is work in progress; you may do with it as you please.

[ussserrr]

Why was this PR suddenly stopped?