GeezerVison

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@notarealdeveloper

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

It would be nice to see a test case added for these methods. If a threading/forking case can be made to pass consistently, that would be nice, but even if not, just a simple exercise of the _at_fork_reinit method would be nice. Then we can make sure there are no reference leaks (with ./python -m test test_whatever -R3:3).

Thanks for the tips! I know exactly what to learn about next. Will push some updates after I learn a bit more. I think the general "unlock the thing" idea probably makes sense, but I'm sure I tripped over myself in 17 different ways in the initial patch. Curious to hear from Victor about whether the overall goal is a sane one. Either way, more coming soon. Happy New Year. :)

Ok, re: ./python -m test some_test -R3:3.

Just updated the pull request with what (I think?) is correct reference counting.

All the tests pass, but I wanted to check for reference leaks like you suggested.

So I ran several of the relevant tests with -R3:3 to check, but then I realized something.

Supposing there's a reference leak, would it be noticed by -R? From the docs, it seems -R uses sys.gettotalrefcount() to check for leaks, but this code is only being executed in the child process created by a fork, so if the parent is checking for updates to a variable in its address space I don't think it'll see them.

Is there a standard way to check for leaks in situations like that, or is it better to just use a brute force strategy like I did in the comment above (i.e., calling every function twice and checking ob_refcnt manually with printf like a caveman).

Hi there. :)

After spending some time thinking about how to exercise the lock re-initialization function in a test, I realized that the best way to overcome my noobish-ness here is to follow the example of _PyImport_ReInitLock, seeing as @vstinner has done a lot of the work on that, and his past fixes of various deadlocks are basically identical to what I'm trying to do here.

The "meat" of the implementation we've already discussed doesn't change (thanks again for the tips @sweeneyde). I've just moved things to different files, in order to maximize the similarity with what _PyImport_ReInitLock does.

Here's what a grep of the codebase for the two functions looks like:

$ grep -Ir _PyImport_ReInitLock
Modules/posixmodule.c:#include "pycore_import.h"        // _PyImport_ReInitLock()
Modules/posixmodule.c:    status = _PyImport_ReInitLock();
Include/internal/pycore_import.h:extern PyStatus _PyImport_ReInitLock(void);
Python/import.c:_PyImport_ReInitLock(void)

$ grep -Ir _PySys_ReInitStdio
Modules/posixmodule.c:#include "pycore_sysmodule.h"     // _PySys_ReInitStdio()
Modules/posixmodule.c:    status = _PySys_ReInitStdio();
Include/internal/pycore_sysmodule.h:extern PyStatus _PySys_ReInitStdio(void);
Python/sysmodule.c:_PySys_ReInitStdio(void)

And here's the git blame output showing the only location where the new function is called.

26881c8fae3 (Victor Stinner  2020-06-02 15:51:37  592)     status = _PyImport_ReInitLock();
26881c8fae3 (Victor Stinner  2020-06-02 15:51:37  593)     if (_PyStatus_EXCEPTION(status)) {
26881c8fae3 (Victor Stinner  2020-06-02 15:51:37  594)         goto fatal_error;
26881c8fae3 (Victor Stinner  2020-06-02 15:51:37  595)     }
26881c8fae3 (Victor Stinner  2020-06-02 15:51:37  596) 
8c8d9c99971 (Jason Wilkes    2022-01-05 11:45:01  597)     status = _PySys_ReInitStdio();
8c8d9c99971 (Jason Wilkes    2022-01-05 11:45:01  598)     if (_PyStatus_EXCEPTION(status)) {
8c8d9c99971 (Jason Wilkes    2022-01-05 11:45:01  599)         goto fatal_error;
8c8d9c99971 (Jason Wilkes    2022-01-05 11:45:01  600)     }

The goal was to get line-by-line correspondence with what @vstinner does with the import lock, and we seem to have that now.

Now, the only difference arises inside the implementation, because the import lock is a global variable inside Python/import.c, while the stdout lock lives on the stdout.buffer object (similarly with stderr). The addition of an _at_fork_reinit method to the buffered object in Modules/_io/bufferedio.c isn't a necessary part of the implementation, it's just the simplest way I knew of (as a noob) to get a pointer to those locks from outside that module. I'd be more than happy to change that detail if anyone thinks there's a better approach.

As for everything else: (1) the PEP 7 formatting should be good now, (2) @the-knights-who-say-ni seem to have given me the "CLA signed" badge just now, (3) in playing with all this, I stumbled on another small unrelated bug in the test suite and fixed it, so I'll submit that as a separate bpo & pull request.

Let me know if there's anything else you'd like me to do. Thanks again for all the help. :)

Re: Helpful comments from @JelleZijlstra

Agree that this needs tests. It also adds a new Python-accessible method. What happens if Python code calls it?

100% agreed, the Python-accessible method wasn't necessary. I needed to learn a bit more before I could do this correctly.

Just pushed some changes to address this. The updated approach:

1. Removes the Python-accessible method.
2. Tests that the lock reinitialization is working as expected (See TestStdioAtForkReInit in Lib/test/test_thread.py).

The test's behavior is to cause the deadlock described in bpo-46210 if and only if the lock is not being reinitialized correctly.

Just like the other C-level locks in the codebase, this is something that should be reinitialized on every fork, so you (@JelleZijlstra) were absolutely right to point out that a Python-accessible interface was an awkward fit.

Further, I want to make it clear to other devs precisely what situation the bugfix is targeting, so the test contains a description of what the bug looks like without the fix, and which specific C function was added to fix it.

Summary: the test can be run with ./python -m test test_thread, and removing the single line status = _PySys_ReInitStdio(); from Modules/posixmodule.c should be sufficient to cause ./python -m test test_thread to fail with a timeout error due to the deadlock, which illustrates how this PR addresses bpo-46210. Let me know if there's anything else you'd like me to do. Thanks again for all the help. :)

In addition to the above:

In the course of adding the test, I found a small bug in the wait_process function in Lib/test/support/__init__.py (the function ignores its timeout argument, and uses a module level global SHORT_TIMEOUT even when timeout is specified).

That won't be relevant to anyone who runs the test as is, since the PR fixes the deadlock.

However, if anyone feels like running the above test after removing the bugfix to verify that the test fails, as it should (if we remove status = _PySys_ReInitStdio(); from Modules/posixmodule.c and then re-run ./python -m test test_thread), then you may notice an odd behavior, in which a line in the new test that says support.wait_process(main_pid, exitcode=0, timeout=5) takes 30 seconds to time out, not 5.

That's due to the above mentioned bug in Lib/test/support/__init__.py:wait_process.

Should I submit another bpo for this small bug, or just send a PR directly?

Should be a one-line fix. Just need to change if dt > SHORT_TIMEOUT in that function to if dt > timeout.

Thanks, can you open another issue for the test.support fix? (I don't have time right now to look at this PR in detail, but I'll review it again.)

Thanks, can you open another issue for the test.support fix? (I don't have time right now to look at this PR in detail, but I'll review it again.)

Will do! :)

Thanks for the help.

Mind if I rebase this on main and force push a small update? I fixed the PR to be consistent with #30928 (bpo-46541).

i.e., stream_at_fork_reinit now uses _Py_ID instead of _Py_IDENTIFIER.

(P.S. Am I imagining things, or do recent changes in main feel very Sam Gross-y? Trying not to get too excited, but I hope that's what's going on. 🎉)

Generally better to merge main, so that the review history gets preserved.

Cool, so this pattern?

1. Merge main (Now "ahead of 'origin/print' by 523 commits.")
2. Cherry pick (Now "ahead of 'origin/print' by 524 commits.")
3. Ok to push, despite the other commits? (i.e., those won't pollute the review history here?)

Apologies for the dumb question. I've used git socially and github personally, but I'm still fairly new to using github socially. :)

Something like this:

• git fetch upstream (assuming upstream is python/cpython)
• git merge upstream/main
• Fix conflicts
• git commit
• git push

GitHub also lets you do this in the web interface if there's a conflict (the "Resolve conflicts" button at the bottom of the thread).

Thanks!