desruisseaux commented on code in PR #399:
URL:
https://github.com/apache/maven-shared-utils/pull/399#discussion_r3702644423
##########
src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java:
##########
@@ -274,34 +274,18 @@ public Integer call() throws CommandLineException {
int returnValue = p.waitFor();
- // TODO Find out if waitUntilDone needs to be called using
a try-finally construct. The method may
- // throw an
- // InterruptedException so that calls to
waitUntilDone may be skipped.
- // try
- // {
- // if ( inputFeeder != null )
- // {
- // inputFeeder.waitUntilDone();
- // }
- // }
- // finally
- // {
- // try
- // {
- // outputPumper.waitUntilDone();
- // }
- // finally
- // {
- // errorPumper.waitUntilDone();
- // }
- // }
- if (inputFeeder != null) {
- inputFeeder.waitUntilDone();
+ try {
+ if (inputFeeder != null) {
+ inputFeeder.waitUntilDone();
+ }
+ } finally {
+ try {
+ outputPumper.waitUntilDone();
+ } finally {
+ errorPumper.waitUntilDone();
+ }
Review Comment:
It works, but if more than one exception is thrown, we see only the last
one. It would be possible to keep all of them with `catch (Throwable e)` and
`Throwable.addSuppressed(e)` calls, but that would make the code more complex
(I'm not sure it would be worth). Alternatively, this exception handling could
be made easier if the pumpers implement `AutoCloseable`.
I'm not really suggesting a change. I'm not against deciding that it is not
worth to keep all exceptions. Just submitting for your consideration and
letting you decide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
