Fix runner hanging, fix an error in thread handling, and serialize concurrent generator-runner invocations #7201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds code to record the thread that currently "owns" or "uses" a ThreadedRunner-instance. This allows to improve the message of the re-entered-RuntimeError. The commit robustifies the generator by ensuring that the exhausted state is actually trivial, i.e. calling send() on an exhausted generator will only use minimal information that is internal to the generator. In addition it tests for None-message in the first send()-call.
This moves the import of datalad.cfg from datalad.cmd.BatchedCommand._get_abandon to the global scope of datalad.cmd. That prevents an error when BatchedCommand instances are deleted during shutdown of python.
This commit adds code to report a situation in which the process does not exist anymore, but a_threaded_runner.process.poll() does return None instead of the return code That might be due to an error in the handling of processes in BatchedCommand. THIS IS A TEMPORARY COMMIT IN ORDER TO WORK OUT THE PROBLEMS
This commit removes the storing of ThreadedRunner instances in WitlessRunner. The stored runner is not used and it is not useful since multiple threads may enter WitlessRunner.run() and overwrite the stored runner in unpredictable ways The commit also improves the description of the `cwd`-parameters and their behavior. The commit fixes a problem with path reporting in CommandErrors which could appear if a non None `cwd` was provided to `WitlessRunner.run()`. I.e. the default `cwd` set in the constructor would have been reported instead of the actual `cwd`, provided when `WitlessRunner.run()` was called. The tests are extended to check for multi-thread support of `WitlessRunner.run()` in the non-generator mode.
This commit blocks a thread that is starting a generator based ThreadedRunner.run, if there is already an active generator. The thread is unblocked, when the active generator is exhausted.
Use python script instead of `cat` to make make tests windows compatible
This commit fixes an error where a process exit was falsly assumed when poll() returned None. That gets rid of the "lost process"-state. Also adapt the tests to the new synchronization in generator runners, i.e. block a new run on an active generator runner until the existing generator is exhausted.
yarikoptic
reviewed
Nov 23, 2022
yarikoptic
reviewed
Nov 23, 2022
yarikoptic
reviewed
Nov 23, 2022
Codecov ReportBase: 88.89% // Head: 90.92% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## maint #7201 +/- ##
==========================================
+ Coverage 88.89% 90.92% +2.03%
==========================================
Files 355 355
Lines 46532 46671 +139
Branches 6332 6348 +16
==========================================
+ Hits 41363 42437 +1074
+ Misses 5154 4219 -935
Partials 15 15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
This commit adds a comment to explain why `ImportError` is caught in SafeDelCloseMixIn.__del__()
This commit filters warnings about unraisable exceptions in `__del__()`, which lead to test errors in some python 3.7 versions.
yarikoptic
reviewed
Nov 25, 2022
yarikoptic
approved these changes
Nov 25, 2022
NP. You are welcome :-) |
|
PR released in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a number of issues.
The PR extends the number of concurrent runner-call scenarios beyond the current implementation, i.e. serialize synchronous (non-generator)
ThreadedRunner.run()-calls. The scenarios now supported are:ThreadedRunner.run()without generator, leading to serialization of invocations.ThreadedRunner.run()with generator, leading to serialisation of invocations, i.e. later invocations return if the currently active generator is exhaustedThreadedRunner.run(). Any number of threads can callsend()on a generator, results will be returned as soon as they are available. Which of the waiting threads is continued is non-deterministic. If the generator is exhausted, all threads will receive aStopIteration-exception.The serialization should fix spurious test errors. Fixes #7138
In addition the PR fixes an error that could lead to hanging threads if
close()ordelis called on aBatchedCommand-instance. This should fix a number of hanging errors in tests that use the@serve_path_via_http()decorator. Fixes #6804