Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Lib/test/test__xxsubinterpreters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, Maybe I am in the wrong way.

def run():
done.set()
interpreters.run_string(interp, dedent(f"""
# wait for "signal"
with open({r}, encoding="utf-8") as rpipe:
Expand All @@ -51,6 +55,7 @@ def run():

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

yield

Expand Down