check for cancellation by callers if mvn launch hangs or takes too long #955

TIBCOeddie · 2022-09-29T20:19:31Z

We ran into a situation where the mvn archetype command was hung (not sure why, but it could happen with network I/O !), and I was surprised to see no cancellation check in a spin-wait loop. Here's a simple change to honor cancellation (hoping it threaded through, but that's also another task)

HannesWell

Thank your for your contribution.
Comment below.

HannesWell · 2022-09-29T22:20:31Z

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGenerator.java

@@ -122,6 +123,12 @@ public Collection<MavenProjectInfo> createArchetypeProjects(IPath location, IArc
          }
        }
        Thread.onSpinWait();
+        // honor cancellations by informing our Future about it then exit 


I suggest to call just subMonitor.checkCanceled(); here and above in the body of the while-loop that calls readAndDispatch().
Then maven.cancel(true); should be called in the catch-clause for CancellationExceptions. In order to have the maven variable available there the code that assigned that variable has to be moved before the try block.

I tried to keep a minimal change.

As I look further, I kind of find little things that would need pulling on.

AbstractProjectScanner has a run declared to throw InterruptedException
use of LocalProjectScanner here catches that and returns an empty list.
What is the intent of that, vs bailing out the way the monitor cancellation is actually handled in this code? (throwing an exception with a Cancel status).

I would like to propose sorting out AbstractProjectScanner, removing the declaration of InterruptedException, (since it has a monitor it does cancellation checks already that way), then handling OperationCancelledException for cancellation... except that this special return of an empty list stands in the way of rationalizing this.

Comments on how to proceed? Do you recognize the advantage or use of an empty list return vs throwing with exception?

Our product uses this ArchetypeGenerator
We obviously already handle CoreException
Common ways of handling a CoreException via ErrorDialog suppress CANCEL which is also good (the user asked for the cancel usually, don't tell them)

so at least in our use case this would be the way to go - all OperationCancelledExceptions turn into a CoreException with status CANCEL

see resulting proposal with 99bddd0

github-actions · 2022-09-29T22:39:11Z

Unit Test Results

596 tests 590 ✔️ 7m 57s ⏱️
  94 suites     6 💤
  94 files     0 ❌

Results for commit a5f6df9.

♻️ This comment has been updated with latest results.

mickaelistria · 2022-09-30T14:38:37Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/project/AbstractProjectScanner.java

+   * 
+   * @param monitor progress monitor, if available, for cancellations and possible progress
+   */
+  public abstract void run(IProgressMonitor monitor);


Unfortunately, removing a throws from an API method can be a breaking change. So please keep the former signature.

Also please note that Thread interruption is not the same as cancellation!
Cancellation is a user action while interruption is the JVMs way to tell a blocking system call that some code tries to interrupt the thread and thus should be (if catched) propagated down to the caller via the interrupt-flag.

Indeed usually this is the case, but I examined AbstractProjectScanner and the only apparent intent seems to be to have honored cancellation. (ie what somebody might write if they missed how eclipse models cancellations and monitors with a runtime exception).

Let me know if your comment was generic, or specific to this use case.

to @mickaelistria indeed it is/would be, however, as I have loaded the entire m2e codebase, the refactoring should be complete. Well, actually, I should say, I observed that it is not a fully breaking change but just a warning by callers about an unnecessary declaration.
see also https://wiki.eclipse.org/Evolving_Java-based_APIs_2 (https://wiki.eclipse.org/Evolving_Java-based_APIs_2#:~:text=Breaks%20compatibility-,(1)%20Adding%20and%20deleting%20checked%20exceptions%20declared%20as%20thrown%20by%20an%20API%20method%20does%20not%20break%20binary%20compatibility%3B%20however%2C%20it%20breaks%20contract%20compatibility%20(and%20source%20code%20compatibility).,-(2)%20Adding%20type)

After you both get a chance to examine this, let me know. Trying to improve the whole codebase (in general, I find progress monitor and cancellation handling paramount when it comes to network-sensitive code (and maven sure loves to hit that network!)).

Note that without expanding the scope of the original commit, there will remain one odd place that clearly is catching a user cancellation (by catching InterruptedException) and yet returns an empty list and does not throw back a cancellation.

as I have loaded the entire m2e codebase

When an API is public, then we can't know of all possible clients. The doc says "Adding and deleting checked exceptions declared as thrown by an API method does not break binary compatibility; however, it breaks contract compatibility (and source code compatibility).".
IMO, it's not worth taking the risk for the moment, we've broken m2e a lot over last release and if we can avoid further incompatibilities for some months, the community will be happier.

understood (I wasn't sure that class was really public; vs inadvertently public for internal use).

let me refine it without touching the API side of things.

OK, please take a peek at 371453f

To be clear, the original issue is quite high (being blocked forever without being able to get out), so do consider landing on whatever release is appropriate.

Should I also rev the plugin that was modified by z+100 ?

HannesWell

One important point regarding the cancellation that is not yet considered, is the termination of the termination of the archetype-generation Maven build.
It looks like it has no effect if the future returned by the EclipseMavenLauncher is canceled. But it should also terminate the process. It is one thing to have a repose in the UI, but if the UI says the operation is canceled the executing process should not continue and maybe some time in the future change something somewhere.

Does anybody now if it is possible to set something like a 'onCancelation' runnable that is executed in the same thread if the Future is canceled? It did not find a handy way for that, but I'm also not familiar with the depths of the Future/CompletionStage API.
If that is feasible the IProcess could be terminated on cancellation. But that would require a moderate amount of restructuring of EclipseMavenLauncher, because the IProcess is not available immediately.

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGenerator.java

laeubi · 2022-10-01T16:31:48Z

You can use the ProgressMonitor to check if it was canceled. Honestly I don't see much value in "cancel the task", if I feel the process takes to long, one can simply terminate the process in the console. This seems the most natural way of "cancel a hung process". For sure one could also think about adding a JobListener or a like to kill the process if it is still running when the job completes.

laeubi · 2022-10-01T16:34:05Z

Does anybody now if it is possible to set something like a 'onCancelation' runnable that is executed in the same thread if the Future is canceled? It did not find a handy way for that, but I'm also not familiar with the depths of the Future/CompletionStage

You can do

completableFuture.whenComplete((result, throwable) -> {
    if (completableFuture.isCancelled()) {
        //do something on cancel...
    }
});

HannesWell · 2022-10-01T22:48:39Z

Does anybody now if it is possible to set something like a 'onCancelation' runnable that is executed in the same thread if the Future is canceled? It did not find a handy way for that, but I'm also not familiar with the depths of the Future/CompletionStage

You can do
completableFuture.whenComplete((result, throwable) -> {
    if (completableFuture.isCancelled()) {
        //do something on cancel...
    }
});

Thanks that worked. But one has to notice that still the original completableFuture has to be used. I first tried to work on the return value of whenComplete().

You can use the ProgressMonitor to check if it was canceled. Honestly I don't see much value in "cancel the task", if I feel the process takes to long, one can simply terminate the process in the console. This seems the most natural way of "cancel a hung process". For sure one could also think about adding a JobListener or a like to kill the process if it is still running when the job completes.

I agree that probably most users will cancel the creation via the console view, since that one pops up automatically and is therefore more prominent. Nevertheless canceling the Future returned by EclipseMavenLauncher should still kill the process. In this case it is an edge case but for the future other use-cases might maybe become relevant where the console is hidden?

laeubi · 2022-10-02T05:46:47Z

Thanks that worked. But one has to notice that still the original completableFuture has to be used. I first tried to work on the return value of whenComplete().

Actually this is a bit an anti-pattern, and whether you use the "original one" or the new completion stage depends on several aspects.

I agree that probably most users will cancel the creation via the console view, since that one pops up automatically and is therefore more prominent. Nevertheless canceling the Future returned by EclipseMavenLauncher should still kill the process. In this case it is an edge case but for the future other use-cases might maybe become relevant where the console is hidden?

CompletableFuture is meant to give you notifications about the process of computing a result, if you are no longer interested, or something goes wrong, it will be canceled and further processing is prevented, but there is no global "cancel" of all resulting completion stages and you can actually have many of them. So I'm not sure if killing the process as a side-effect of canceling the computation is valid in general, at least it mus be clearly documented.

TIBCOeddie · 2022-10-07T18:03:42Z

Is this good to go as-is? Let's not let perfect stop good enough...

HannesWell

I'm fine with this as it is now.
Terminating the process if the future is canceled still seems reasonable for me, but I think it is not urgent because as said by @laeubi most of the time the creating Maven-Process will be terminated instead of the job. So that can be done later (or not), if necessary.

@laeubi are you fine with that as well?

@TIBCOeddie can you please rebase and force push on the current master?

TIBCOeddie · 2022-10-10T17:19:08Z

@TIBCOeddie can you please rebase and force push on the current master?

I'm fully up to date near as I can tell. Just blocked on the approvals?

HannesWell · 2022-10-10T22:46:19Z

I rebased this commit manually (merge commits usually cause problems due to the tycho-p2-plugin).
@laeubi any objections? Otherwise I will merge this as it is.

HannesWell · 2022-10-11T18:23:55Z

Thank you @TIBCOeddie for your contribution.

HannesWell requested changes Sep 29, 2022

View reviewed changes

mickaelistria reviewed Sep 30, 2022

View reviewed changes

HannesWell requested changes Oct 1, 2022

View reviewed changes

org.eclipse.m2e.core.ui/src/org/eclipse/m2e/core/ui/internal/archetype/ArchetypeGenerator.java Show resolved Hide resolved

TIBCOeddie requested a review from HannesWell October 3, 2022 14:22

HannesWell approved these changes Oct 8, 2022

View reviewed changes

HannesWell requested a review from laeubi October 10, 2022 17:23

Check for cancellation by callers if mvn launch hangs or takes too long

a5f6df9

HannesWell force-pushed the master branch from 6768220 to a5f6df9 Compare October 10, 2022 22:09

laeubi approved these changes Oct 11, 2022

View reviewed changes

HannesWell merged commit 57d17d0 into eclipse-m2e:master Oct 11, 2022

HannesWell added this to the 2.1.0 milestone Oct 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for cancellation by callers if mvn launch hangs or takes too long #955

check for cancellation by callers if mvn launch hangs or takes too long #955

TIBCOeddie commented Sep 29, 2022

HannesWell left a comment

HannesWell Sep 29, 2022

TIBCOeddie Sep 30, 2022

TIBCOeddie Sep 30, 2022

github-actions bot commented Sep 29, 2022 •

edited

Loading

mickaelistria Sep 30, 2022

laeubi Sep 30, 2022

TIBCOeddie Sep 30, 2022 •

edited

Loading

mickaelistria Sep 30, 2022

TIBCOeddie Sep 30, 2022

TIBCOeddie Sep 30, 2022

HannesWell left a comment

laeubi commented Oct 1, 2022

laeubi commented Oct 1, 2022

HannesWell commented Oct 1, 2022

laeubi commented Oct 2, 2022

TIBCOeddie commented Oct 7, 2022

HannesWell left a comment

TIBCOeddie commented Oct 10, 2022

HannesWell commented Oct 10, 2022

HannesWell commented Oct 11, 2022

check for cancellation by callers if mvn launch hangs or takes too long #955

check for cancellation by callers if mvn launch hangs or takes too long #955

Conversation

TIBCOeddie commented Sep 29, 2022

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIBCOeddie Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

laeubi commented Oct 1, 2022

laeubi commented Oct 1, 2022

HannesWell commented Oct 1, 2022

laeubi commented Oct 2, 2022

TIBCOeddie commented Oct 7, 2022

HannesWell left a comment

Choose a reason for hiding this comment

TIBCOeddie commented Oct 10, 2022

HannesWell commented Oct 10, 2022

HannesWell commented Oct 11, 2022

github-actions bot commented Sep 29, 2022 •

edited

Loading

TIBCOeddie Sep 30, 2022 •

edited

Loading