Use a separate ExecutorService in AsyncCompletionProposalPopup

[fedejeanne]

Tip

I'm targeting this PR for M1 so I'll keep it drafted. Feel free to review and test anyway.

What it does

Use a dedicated ExecutorService in AsyncCompletionProposalPopup so that canceling the futures works as expected.

Before this PR, the method AsyncCompletionProposalPopup.cancelFutures() did not stop running futures, more precisely: it didn't stop the computationFutures. This was because the common pool was being used.

This PR introduces a dedicated executor service (with its own pool) that can be shut down immediately when the completion popup is closed.

[laeubi]

@fedejeanne why I would not object to use an own pool, I still wonder why the features can not be canceled? Also using a single threadpool is different from using the common pool because it guarantees some kind of parallelism.

[fedejeanne]

@laeubi there seems to be no guarantee that the futures will be canceled immediately if mayInterruptIfRunning is set to true. This is from the JavaDocs:

mayInterruptIfRunning - this value has no effect in the default implementation because interrupts are not used to control cancellation.

There are some more details in this stackoverflow post

[github-actions]

Test Results

 2 778 files  ±0   2 778 suites  ±0   1h 41m 36s ⏱️ +52s
 7 932 tests ±0   7 700 ✅  - 3  228 💤 ±0  4 ❌ +3 
23 349 runs  ±0  22 595 ✅  - 7  746 💤 ±0  8 ❌ +7 

For more details on these failures, see this check.

Results for commit 8fbd0ed. ± Comparison against base commit a9b8d9a.

♻️ This comment has been updated with latest results.

[laeubi]

@laeubi there seems to be no guarantee that the futures will be canceled immediately if mayInterruptIfRunning is set to true. This is from the JavaDocs:

mayInterruptIfRunning - this value has no effect in the default implementation because interrupts are not used to control cancellation.

There are some more details in this stackoverflow post

But there is also no guarantee that the executor is terminating immediately. Request for cancel is always only a best effort and it depends on the code to handle it properly, so the question would be what exact problem is here to be solved? So I suspect some code currently checks for Thread.interrupted() should instead check for CompletableFuture.isCancelled() instead.

[fedejeanne]

@laeubi , sorry, some context is still missing. I was in the process of creating other PRs that are related to this one:

• Don't log InterruptedExceptions in CompletionProposalComputerDescriptor eclipse-jdt/eclipse.jdt.ui#2425
• Check for thread interruption in Parser::parse eclipse-jdt/eclipse.jdt.core#4302

You are right: cancellation is done in a best effort and someone (the task) needs to check for cancellation on its own. That one is done in eclipse-jdt/eclipse.jdt.core#4302 and I've added some details in there (see How to test) explaining how all 3 PRs play together. Does this help?

[fedejeanne]

Test failures were already there: #3155

[laeubi]

@fedejeanne I'll try to look into this in more details but will be unavailable for a few days now. just some hints here:

• in Don't log InterruptedExceptions in CompletionProposalComputerDescriptor eclipse-jdt/eclipse.jdt.ui#2425 you see we have a ProgressMonitor, cancellation is usually handled through that pattern in Eclipse code
• in the context of features, one would likely pass a ProgressMonitor that in it isCanceled() method checks for the cancellation of the feature (similar to this one here that cancel on first match found: https://github.com/eclipse-pde/eclipse.pde/blob/e7b608f63275301b95614581c3462c4bb04b305e/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/search/dependencies/GatherUnusedDependenciesOperation.java#L431-L443
• Similar one might want to interrupt the current thread of an operation running in a feature if it is required on cancellation instead of throw away the whole executor

So I think we should really see if it can be solved without shutdown the whole pool (what makes a pool quite useless here) if we just want to cancel some of the features.

[fedejeanne]

@laeubi I also thought of using a progress monitor, as it is the "standard" way of doing it RCP, but the thing is that the method I want to reach (Parser::parse) is too deep in the call hierarchy. You'll notice that org.eclipse.jdt.core.compiler.batch has no dependency to any other bundles... tricky.

[laeubi]

Some one is calling this code and someone is canceling the future :-)
So somewhere there I would expect to cancel the feature and the thread (if not other way is there). But thread cancellation is something that is poorly supported by the platform code anyways so you will probably get some surprise.

[fedejeanne]

?

Sorry, you'll have to lay it down a bit differently because you got me confused 😅

... the code is called and canceled in AsyncCompletionProposalPopup:

• Called (started) in: buildCompletionFuturesOrJobs(...) (see CompletableFuture.supplyAsync(() -> { ... }, completionFuturesExecutor));)
• Canceled in: cancelFutures() (when the user clicks outside the completion proposals pop-up)

Isn't that what you mean?