bpo-37224: Using threading.Event to make sure the thread is running already.

[shihai1991]

https://bugs.python.org/issue37224

[pablogsal]

I don't think this is fixing anything, unfortunately: reading for a pipe is a blocking operation until someone has written to it so if IIC the thread will actually block on rpipe.read() until the main thread writes to it, so the event is not synchronizing any resource.

[pablogsal]

Maybe this is working but then I don't understand how :(

[shihai1991]

Hi, Pablo. Thanks for your quick review :)

IMO, done.set() is different with rpipe.read(). Why I am use the done.set() in here?
the reason is I get some fail info from this test case:

======================================================================                        
FAIL: test_already_running (test.test__xxsubinterpreters.RunStringTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shihai/cpython/Lib/test/test__xxsubinterpreters.py", line 831, in test_already_running
      with self.assertRaises(RuntimeError):
AssertionError: RuntimeError not raised

This failed test case shows that the subinterp don't running immedately in _running(). So I guess that the thread can't get the GIL in time in C level :)

[pablogsal]

Thanks for the response, @shihai1991.

IMO, done.set() is different with rpipe.read().

I understand that. But I don't think you are addressing my question:

reading for a pipe is a blocking operation until someone has written to it so if IIC the thread will actually block on rpipe.read() until the main thread writes to it, so the event is not synchronizing any resource.

You mentioned that:

the reason is I get some fail info from this test case:

But you are not explaining why your solution works or how is avoiding the problem. pipe.read() is a blocking call that will wait for the other side to write to the Pipe. Also, even if this event works, there is still the possibility that the event is set and you get to write before the thread even opens the pipe on the other side, so even if this fix is covering a race, is not fixing it entirely.

This failed test case shows that the subinterp don't running immedately in _running(). So I guess that the thread can't get the GIL in time in C level :)

What does that mean? Either it gets the GIL or it doesn't and if it doesn't another thread will run in the middle.

[shihai1991]

I understand that. But I don't think you are addressing my question:

reading for a pipe is a blocking operation until someone has written to it so if IIC the thread will actually block on rpipe.read() until the main thread writes to it, so the event is not synchronizing any resource.

Oh. Sorry. I didn't explain it clearly. event have been set before running the subinterp, right?

But you are not explaining why your solution works or how is avoiding the problem. pipe.read() is a blocking call that will wait for the other side to write to the Pipe.

I have no objection to that. I use event.set() to make sure the thread is already running in C level.

Also, even if this event works, there is still the possibility that the event is set and you get to write before the thread even opens the pipe on the other side, so even if this fix is covering a race, is not fixing it entirely.

Sorry, I don't know how it's happen. yield make sure the pipe.read() before pipe.write(), no?

What does that mean? Either it gets the GIL or it doesn't and if it doesn't another thread will run in the middle.

I want express that we can't assure the thread will run immediately.

[pablogsal]

. I use event.set() to make sure the thread is already running in C level.

I don't understand what you mean by that.

In short, unfortunately I'm still very confused about what this PR is achieving. Maybe @vstinner or someone else have a different view and can explain better

[shihai1991]

OK, Maybe I am in the wrong way.

[shihai1991]

Hi, Victor, Eric. Would you mind to take a look this PR? @vstinner @ericsnowcurrently

I don't catch the error again by ./python -m test test__xxsubinterpreters -j250 -F -v over 11 hours in my local vm.

[shihai1991]

In fact, there have two types of error:

1. test_already_running: can't get the interp even it have been created in setup().
2. test_still_running: interp isn't running immediately.

[ericsnowcurrently]

@shihai1991, I'll take a look when I get a chance.

[shihai1991]

@shihai1991, I'll take a look when I get a chance.

Ok. Thanks, Eric. Looks like I need to get to the bottom of this. I get this error again :(

test_already_running (test.test__xxsubinterpreters.RunStringTests) ... spam
[InterpreterID(0), InterpreterID(60)]
FAIL

======================================================================
FAIL: test_already_running (test.test__xxsubinterpreters.RunStringTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shihai/cpython/Lib/test/test__xxsubinterpreters.py", line 836, in test_already_running 
    with self.assertRaises(RuntimeError):
AssertionError: RuntimeError not raised

---------------------------------------------------------------------- 

Ran 123 tests in 111.315s

FAILED (failures=1, skipped=6)
Warning -- Uncaught thread exception: RuntimeError
Exception in thread Thread-8 (run):
Traceback (most recent call last):
  File "/home/shihai/cpython/Lib/threading.py", line 1006, in _bootstrap_inner
    self.run()
  File "/home/shihai/cpython/Lib/threading.py", line 943, in run
    self._target(*self._args, **self._kwargs)
  File "/home/shihai/cpython/Lib/test/test__xxsubinterpreters.py", line 50, in run
    interpreters.run_string(interp, dedent(f"""
RuntimeError: unrecognized interpreter ID 60
test test__xxsubinterpreters failed

my second patch is:

diff --git a/Lib/test/test__xxsubinterpreters.py b/Lib/test/test__xxsubinterpreters.py
index 7baea69a4e5..2b2a3d4bd7d 100644
--- a/Lib/test/test__xxsubinterpreters.py
+++ b/Lib/test/test__xxsubinterpreters.py
@@ -42,8 +42,13 @@ def _run_output(interp, request, shared=None):
 @contextlib.contextmanager
 def _running(interp):
     r, w = os.pipe()
+    # [bpo-37224](https://bugs.python.org/issue37224): Using threading.Event to make sure
+    # the interpreter is running already.
+    done = threading.Event()
+    print(interpreters.list_all())
     def run():
         interpreters.run_string(interp, dedent(f"""
+            {done.set()}
             # wait for "signal"
             with open({r}, encoding="utf-8") as rpipe:
                 rpipe.read()
@@ -51,6 +56,7 @@ def run():

     t = threading.Thread(target=run)
     t.start()
+    done.wait()

     yield

[github-actions]

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

[shihai1991]

This PR hasn't fix the failed test cases, I updated the analysis conclusion in https://bugs.python.org/issue37224#msg397809.
And created a new PR in #27240 .