You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by jaikiran <gi...@git.apache.org> on 2017/12/16 10:18:31 UTC
[GitHub] ant pull request #52: [master] Fix BZ-58451 BZ-58833
GitHub user jaikiran opened a pull request:
https://github.com/apache/ant/pull/52
[master] Fix BZ-58451 BZ-58833
The commit here fixes the issues reported in:
https://bz.apache.org/bugzilla/show_bug.cgi?id=58833
https://bz.apache.org/bugzilla/show_bug.cgi?id=58451
The `PumpStreamHandler` and the `StreamPumper` work together for redirecting the streams (reading from inputstream and writing to a outputstream). When the `StreamPumper` is asked to stop, it stops reading from the input stream and goes on to finish any (non-blocking) reads and finally a (potentially blocking) flush of whatever it has read so far. The `PumpStreamHandler`, which initiates that `stop` is a bit too impatient (mainly because of its inability to know what's going on in `StreamPumper` after the `stop` was invoked on it) and starts interrupting the `StreamPumper`'s thread very soon (as early as 200 milli seconds after stop). This triggers a bunch of issues in `StreamPumper`, which either is in the middle of finishing the non-blocking reads or in the middle of actually flushing out whatever it's held on to and ultimately leads to a non-clean stop and thus corrupting/truncating the output stream as noted in those issues.
The commit here introduces a way, through the `PostStopHandle`, for `PumpStreamHandler` to be aware that upon `stop` the `StreamPumper` has respected the call to `stop` and is doing certain post-stop finishing acts before actually being considered `finished`. The `PumpStreamHandler` then allows a bit of time for the post-stop tasks to finish before actually trying to kill off the `StreamPumper` using thread interrupts, if the `StreamPumper` didn't finish after that grace period.
The commit here intentionally does _not_ introduce a configurable "grace period" for the post-stop activities and instead using a hard coded (maximum) 2 seconds for the following reasons:
* The 2 seconds is the _maxium_ amount of time to let the post-stop to complete, so it's not always going to wait that long
* The cleaning up gracefully is an internal implementation detail and doesn't have to end up being configured by the user.
* Finally, the `PumpStreamHandler` anyway falls back to the thread interrupts if the post-stop (flush() etc...) doesn't complete in those 2 seconds. So from a end user point of view, there isn't really any behaviour change, except that we now give the `StreamPumper` a chance to gracefully stop.
This PR is against master branch. If this is approved I'll backport it to 1.9.x branch too.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jaikiran/ant bz-58451-master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/ant/pull/52.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #52
----
commit 3bcdac90581cb78e4a49b0b7fe6b01657a6436f1
Author: Jaikiran Pai <ja...@apache.org>
Date: 2017-12-16T09:48:30Z
BZ-58451 BZ-58833 Give StreamPumper a chance to finish cleanly before interrupting its thread, to prevent truncated output
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/9/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/8/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant pull request #52: [master] Fix BZ-58451 BZ-58833
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran closed the pull request at:
https://github.com/apache/ant/pull/52
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:
https://github.com/apache/ant/pull/52
Have updated the PR to fix the test case so that it runs the correct command on Windows, during the tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/21/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/15/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/16/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/14/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:
https://github.com/apache/ant/pull/52
Thanks Stefan (@bodewig) for the review. The tests (existing and a new one in this PR) are passing on both *nix and Windows. Given that this change does address the issues noted in the referenced bugzilla issues (tested both manually and automated), I will go ahead and merge this change and keep a watch on any issues in and around this area if/when they get reported.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:
https://github.com/apache/ant/pull/52
@bodewig, do you think this change is OK? Any thoughts/concerns? I understand that this is one of the central classes, so don't want to push this if there's any concerns with this change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/15/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/52
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/10/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:
https://github.com/apache/ant/pull/52
This is now commit to both master and 1.9.x branches https://github.com/apache/ant/commit/6c860e0036a8742f4cc35e3e6a8e595e42804c89
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #52: [master] Fix BZ-58451 BZ-58833
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/ant/pull/52
I must admit that I'm scared of every change in this area, we've found and fixed too many subtle bugs in code that looked right.
Your change looks very reasonable and if all tests pass on Windows as well, then I'm fine with it (but still scared :-).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org