Remove thread stop #5315
Merged
Remove thread stop #5315
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
These stop calls were put in place during a time when the "pump" threads were not easily interrupted. Usually this was due to being forced to work with opaque IO streams rather than the actual channels. At this point, with Thread#stop going away and still causing log noise, we have decided to remove all calls. This code was used primarily by the few remaining non-popen-based command launches (usually which read the streams to completion and do not need to be interrupted), and for much of IO on Windows (since we have not finished porting native IO logic to Windows). If the removal of these calls leads to leaky pump threads, we will find an alternative way or prioritize the completion of native IO on Windows.
headius
force-pushed
the
remove_thread_stop
branch
from
38901b8
to
79b6b33
Compare
enebo
approved these changes
Sep 19, 2018
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 is to resolve #1183 by replacing the remaining stop calls with interrupt calls.
This may lead to bad behavior in the threads we formerly forced to stop, but it would be better to know that these cases exist than continue to call this bad method.