gh-109370: Fix unexpected traceback output in test_concurrent_futures #109780
Conversation
…utures Follow-up of pythongh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED.
serhiy-storchaka
added
type-bug
| @@ -521,7 +521,8 @@ def terminate_broken(self, cause): | |||
|
|
|||
| # gh-107219: Close the connection writer which can unblock | |||
| # Queue._feed() if it was stuck in send_bytes(). | |||
| self.call_queue._writer.close() | |||
| if sys.platform == 'win32': | |||
There was a problem hiding this comment.
Why is it an issue on other platforms?
See also PR gh-109810 which goes further in terminate_broken(), to try harder to close everything, and terminate the child processes.
There was a problem hiding this comment.
Because of scenarios described in #109397 (comment). It is not possible to safely close a file descriptor in other thread without using some kind of locking, and it is difficult if possible without rewriting the way the GIL or all IO works even to use locking.
It may be lesser issue on Windows (if handles are never reused, I do not know if it is so), but I am not sure that it is bug-free there. It may be simply that bugs are rarely manifested on Windows because most time the queue thread spends in WaitForMultipleObjects().
There was a problem hiding this comment.
It is not possible to safely close a file descriptor in other thread without using some kind of locking
It's maybe time to add "some kind of locking".
There are two cases:
-
Thread B sets the file descriptor / handle to None, and then close FD / handle. No other thread was using it, no problem. Next, when thread A tries to use it, "closed FD/handle" exception is raised.
-
Thread B closes the FD/handle while thread A is using it. That's an important use case to be able to unblock multiprocessing when it goes into troubles with a sick worker progress, sick manager process, well, when things "go wrong".
For me, the case that we should care about is terminate_broken() of concurrent.futures ProcessPoolExecutor and the associated BrokenProcessPool exception. When everything goes wrong, we should just cleanup resources: terminate processes, close queue, close file descriptors/handles, etc. I don't think that we can still expect worker processes to handle "please stop" commands from the main process.
For example, PR #109810 is when the executor cannot even create new threads. In that case, multiprocessing and concurrent.futures cannot work properly, and it's time to exit as soon as possible.
There was a problem hiding this comment.
It's maybe time to add "some kind of locking".
Actually, I do not think how is it possible, even with cooperation with GIL. It needs a support in the kernel to allow switching to other thread only when a system call waits. We can reduce the window for race condition, but it will always be possible with some small probability to close and replace file descriptor just before system call.
There are two cases:
There are many other cases, depending on definition of "use it". If the file descriptor is closed or the attribute is set to None between the place where you can realistically check this and the place where it is used, you have an unsolvable problem.
For now, I am worried about regression introduce by #107219 on non-Windows platforms. Since the original issue was Windows-only, I prefer to apply the solution (even if it may be non-perfect) on Windows where it fixes serious issue and not apply it other platforms where it seems unnecessary but causes visible regression.
There was a problem hiding this comment.
LGTM.
Honestly, I don't understand well this code. Apparently, there are multiple known issues. There is no simple fix. This change restores the code to the previous "status quo" and keep my fix for the other issue. It sounds like an acceptable tradeoff.
Thanks for reusing ERROR_OPERATION_ABORTED macro, I dislike hardcoded constants :-)
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
…utures (pythonGH-109780) Follow-up of pythongh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED. (cherry picked from commit 0b4e090) Co-authored-by: Serhiy Storchaka <[email protected]>
|
GH-109882 is a backport of this pull request to the 3.11 branch. |
…futures (GH-109780) (GH-109882) Follow-up of gh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED. (cherry picked from commit 0b4e090) Co-authored-by: Serhiy Storchaka <[email protected]>
…utures (pythonGH-109780) Follow-up of pythongh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED.
@serhiy-storchaka This fix has been merged to |
…rrent_futures (pythonGH-109780) Follow-up of pythongh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED. (cherry picked from commit 0b4e090) Co-authored-by: Serhiy Storchaka <[email protected]>
|
GH-111934 is a backport of this pull request to the 3.12 branch. |
|
Thank you for your reminder @hugovk. |
…utures (pythonGH-109780) Follow-up of pythongh-107219. * Only close the connection writer on Windows. * Also use existing constant _winapi.ERROR_OPERATION_ABORTED instead of WSA_OPERATION_ABORTED.
Follow-up of gh-107219.