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

shihai1991 · 2021-06-08T11:12:35Z

https://bugs.python.org/issue37224

pablogsal · 2021-06-08T11:37:54Z

Lib/test/test__xxsubinterpreters.py

@@ -42,7 +42,11 @@ def _run_output(interp, request, shared=None):
 @contextlib.contextmanager
 def _running(interp):
    r, w = os.pipe()
+    # bpo-37224: Using threading.Event to make sure
+    # the thread is running already.
+    done = threading.Event()


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.

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

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 :)

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.

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.

. 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

OK, Maybe I am in the wrong way.

shihai1991 · 2021-06-08T11:38:21Z

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 · 2021-06-08T15:43:46Z

In fact, there have two types of error:

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

ericsnowcurrently · 2021-06-09T20:49:48Z

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

shihai1991 · 2021-06-10T11:31:21Z

@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 · 2021-07-16T00:04:16Z

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

shihai1991 · 2021-07-19T16:09:10Z

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 .

Using threading.Event to make sure the thread is running already.

202ba49

the-knights-who-say-ni added the CLA signed label Jun 8, 2021

bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Jun 8, 2021

shihai1991 added the skip news label Jun 8, 2021

pablogsal reviewed Jun 8, 2021

View reviewed changes

shihai1991 requested review from vstinner and ericsnowcurrently June 8, 2021 12:06

github-actions bot added the stale Stale PR or inactive for long period of time. label Jul 16, 2021

shihai1991 closed this Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shihai1991 commented Jun 8, 2021 •

edited by bedevere-bot

Loading

pablogsal Jun 8, 2021

pablogsal Jun 8, 2021

shihai1991 Jun 8, 2021

pablogsal Jun 8, 2021

shihai1991 Jun 8, 2021

pablogsal Jun 8, 2021

shihai1991 Jun 8, 2021

shihai1991 commented Jun 8, 2021 •

edited

Loading

shihai1991 commented Jun 8, 2021

ericsnowcurrently commented Jun 9, 2021

shihai1991 commented Jun 10, 2021 •

edited by bedevere-bot

Loading

github-actions bot commented Jul 16, 2021

shihai1991 commented Jul 19, 2021

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

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

Conversation

shihai1991 commented Jun 8, 2021 • edited by bedevere-bot Loading

pablogsal Jun 8, 2021

Choose a reason for hiding this comment

pablogsal Jun 8, 2021

Choose a reason for hiding this comment

shihai1991 Jun 8, 2021

Choose a reason for hiding this comment

pablogsal Jun 8, 2021

Choose a reason for hiding this comment

shihai1991 Jun 8, 2021

Choose a reason for hiding this comment

pablogsal Jun 8, 2021

Choose a reason for hiding this comment

shihai1991 Jun 8, 2021

Choose a reason for hiding this comment

shihai1991 commented Jun 8, 2021 • edited Loading

shihai1991 commented Jun 8, 2021

ericsnowcurrently commented Jun 9, 2021

shihai1991 commented Jun 10, 2021 • edited by bedevere-bot Loading

github-actions bot commented Jul 16, 2021

shihai1991 commented Jul 19, 2021

shihai1991 commented Jun 8, 2021 •

edited by bedevere-bot

Loading

shihai1991 commented Jun 8, 2021 •

edited

Loading

shihai1991 commented Jun 10, 2021 •

edited by bedevere-bot

Loading