GeezerVison

@cjerdonek Are you still interested/able to work on this? I'm mainly asking since it's been a few months since there was activity, and I'm fairly confident that my suggestion will resolve the CI failures. If you're not currently able to, I can re-open a separate PR applying this changes in addition to mine with you as the co-author, titling it something like "fix MultiLoopChildWatcher and ThreadChildWatcher hangs".

@vstinner recently brought to my attention that this issue has some degree of urgency, with it having caused intermittent failures in the buildbots for quite some time now. So, there's some definitely an advantage to having it resolved sooner than later.

@aeros As I said in the reply above to the review comment, I think the tearDown() change I made here shouldn't be done in this PR (or in any PR in combination with a fix for the hang). It should be incidental to the MultiLoopChildWatcher hang and is a separate issue (if it is an issue).

Regarding the rest of the PR, sure, you're welcome to continue what I started. However, I definitely don't think the changes I made to MultiLoopChildWatcher in this PR should be made as is. This was just a minimal start that applied what I found and that I confirmed addressed the hang. So while the general direction of the solution and PR should be sound (i.e. ensuring that the wakeup file descriptor is set in attach_loop() as I mentioned in the bug report), there are other aspects that need to be addressed.

For example, I don't think attach_loop() should be setting self._loop as I did. And there's a chance it may need to support being called on more than one loop, which means in turn that close() will need to be able to unwind the operation for each loop that attach_loop() was called on. So there's a chance you will need to record in an attribute the set of loops that attach_loop() was called on. I stopped working on the PR before I started thinking through these other issues.

Something else to be aware of is that MultiLoopChildWatcher has this code comment right after its docstring:

# Implementation note:
# The class keeps compatibility with AbstractChildWatcher ABC
# To achieve this it has empty attach_loop() method
# and doesn't accept explicit loop argument
# for add_child_handler()/remove_child_handler()
# but retrieves the current loop by get_running_loop()

The part mentioning its "empty attach_loop()" method should probably be updated. I'm not sure why it says that and what it means (since its attach_loop() method isn't currently empty).

The part mentioning its "empty attach_loop()" method should probably be updated. I'm not sure why it says that and what it means (since its attach_loop() method isn't currently empty).

From the git blame, it looks like Andrew wrote that note when initially implementing the MultiLoopoChildWatcher class in 0d671c0. @asvetlov, could you clarify?

I reverted the tearDown() change. We'll see what the tests do.

Can I suggest an improved documentation-comment?

class MultiLoopChildWatcher(AbstractChildWatcher):
    """A watcher that allows running event loops outside of the main thread.
    
    This implementation registers a SIGCHLD handler in the event loop of
    the main thread.  It requires the main thread use an asyncio event loop,
    and it will be affected by anything that delays or blocks event processing
    in the main thread.  It will conflict with any other code that installs its own
    signal handler for SIGCHLD.

I noticed a similar constraint in the existing code. MultiLoopChildWatcher could cause hangs if the main thread is waiting in a system call. Because the watcher's signal handler would not get run, until the system call returns. It only works properly, if the main thread can be woken up by [EDIT: a signal that happens to be received on the main thread, or] the fd passed to signal.set_wakeup_fd().

This PR tightens the requirements slightly. MultiLoopChildWatcher now needs the main thread to run an asyncio event loop. MultiLoopChildWatcher will cause hangs if the main thread waits elsewhere in a system call. Or if the main thread runs a very long computation without returning to the event loop.

(Code looks plausible. Good analysis of a nasty problem. The signal arrives e.g. within the libc select() function. The python signal handler, MultiLoopChildWatcher._sig_chld(), doesn't get a chance to run before the low-level system call.

signal.set_wakeup_fd() is the solution. It gets the select() system call to wake up. The python-level SIGCHLD handler gets a chance to run.).

I would drop self._saved_sighandler if I could. This is getting messy. I'm a bit surprised to think of code that contrives to work that way. I.e. that temporarily uses asyncio.subprocess, doesn't need their own SIGCHLD handler to work for the duration, but needs it to start working again afterwards. It warned this would conflict from the start, and it was only introduced in 3.8...).

Thanks @sourcejedi for the second pair of eyes and great additional comments! I like your doc suggestion and agree it should be incorporated in some form into this PR.

A couple unrelated things:

• I filed a trivial refactoring PR bpo-38323: Add guard clauses in MultiLoopChildWatcher. #22756 to precede the changes here. It will make the changes here smaller and make the diff easier to understand / review.

• I noticed that AbstractChildWatcher.attach_loop() has this documentation:

  If the watcher was previously attached to an event loop, then it is first detached before attaching to the new loop.

  So the current PR will also need to be adjusted to handle the loop being different if attach_loop() was already called.

I just noticed that changing MultiLoopChildWatcher to require a loop may be the same as turning MultiLoopChildWatcher into SafeChildWatcher:
https://docs.python.org/3/library/asyncio-policy.html#asyncio.SafeChildWatcher

I guess this means MultiLoopChildWatcher should be changed to use signal.set_wakeup_fd(), but not via a loop's loop.add_signal_handler().

@asvetlov "MultiLoopChildWatcher problems can and should be fixed; there is nothing bad in the idea but slightly imperfect implementation."

This fix needs to switch from using signal.signal(), to loop.add_signal_handler(). This requires the main thread to run an asyncio event loop. Previously, this requirement was only documented for SafeChildWatcher and FastChildWatcher. So we need to decide -

Q. Is it acceptable to add this new requirement? Or do we deprecate MultiLoopChildWatcher, and warn people that it occasionally hangs?

In other words, it is possible an existing program used the "imperfect" MultiLoopChildWatcher, while the main thread ran a non-asyncio event loop. (And the non-asyncio event loop was using signal.set_wakeup_fd()). This fix would break any such program (hang in await subprocess.wait()).

signal.set_wakeup_fd() is roughly the only way python programs can use signal handlers correctly. The python-level signal handler is run in the main thread. If the main thread is waiting in a system call, it needs something to wake it up after a signal was delivered to a different thread. POSIX says that process signals can be delivered to any thread.

(I say "process signals" to exclude pthread_kill(), and faults like SIGSEGV. Most programs should assume they have some threads started by random library code - see e.g. ThreadedChildWatcher :-P).

I guess this means MultiLoopChildWatcher should be changed to use signal.set_wakeup_fd(), but not via a loop's loop.add_signal_handler().

I don't know how that would help, sorry. We absolutely need to interrupt the epoll_wait() call in the main thread. We need that epoll to be watching the signal wakeup fd. Hence we need to talk to the main thread event loop. We may as well use loop.add_signal_handler().

I said that because if MultiLoopChildWatcher is changed to require an event loop to be running in the main thread, then it will be identical to SafeChildWatcher. So I don't think that change should be made. Indeed, looking at MultiLoopChildWatcher's docstring, that looks to be its distinguishing feature:

So I was wondering if there might be any options for using signal.set_wakeup_fd() that don't require running a full-blown asyncio event loop. (MultiLoopChildWatcher could do any setup it requires.)

In the meantime, we might also want to think about whether a band-aid could be added to the tests, at least to prevent hangs as long as MultiLoopChildWatcher stays the same.

Yeah, I would agree that changing MultiLoopChildWatcher to require being used in the main thread with a running event loop would definitely defeat the main purpose of it.

In the meantime, we might also want to think about whether a band-aid could be added to the tests, at least to prevent hangs as long as MultiLoopChildWatcher stays the same.

My only concern though is that by bandaging the tests temporarily, it may encourage usage in production in situations that could lead to the hangs (although admittedly, I'm not aware of specific real-world situations where it would currently be used). That being said, it's seeming like we may have to go that route for now since this issue seems like it might be blocked on feedback from @asvetlov (since it seems like it might require some fundamental design changes). This would at least ensure that other test failures aren't obfuscated due to intermittent MultiLoopChildWatcher hangs.

My only concern though is that by bandaging the tests temporarily, it may encourage usage in production in situations that could lead to the hangs

I was just meaning in master (e.g. for the next few months) while a resolution is developed. By the way, @aeros can you review my other PR #22756, which is independent of (but related to) this issue?

I'm closing this PR which was mainly exploratory. See here for my conclusions (also noted above): https://bugs.python.org/issue38323#msg395113