Fix UI flickering in code mining by adding wait/notify reconciliation coordination #2698

tobiasmelcher · 2025-12-15T17:41:24Z

Problem: Previous implementation returned emptyList() immediately when reconciliation wasn't complete, causing code minings to disappear/reappear during AST reconciliation → UI flickering

Solution:

Add shared RECONCILE_LOCK and reconciledViewers Set for coordination
Code mining provider now waits (up to 30s) for reconciliation to complete
Reconciler signals completion with notifyAll() in reconciled()
Remove viewer from reconciled set in aboutToBeReconciled() during edits

This pull request will solve following reported issue eclipse-platform/eclipse.platform.ui#2786 (comment)

jjohnstn

Works as stated and logic seems reasonable. I noticed in eclipse-platform/eclipse.platform.ui#2786 (comment) that you mention that you aren't sure about waiting for reconcile to finish and @laeubi states that waiting is never a good idea with a CompletableFuture, however, this patch does wait in the CompletableFuture. I didn't see you respond to @laeubi's comment. Can you comment here?

mickaelistria · 2025-12-16T07:52:54Z

That seems like a good change

laeubi · 2025-12-16T07:59:09Z

@jjohnstn yes that's right. But on the other hand of course in a legacy code base one might need to do it incrementally. To explain the problem here what will happen is that there is a thread allocated from the common pool just for waiting. This decreases the possible concurrency level and the jvm will allocate possibly more threads... not so good but I would assume that reconciliation is hopefully fast, 30 seconds sounds way to much. Also keep in mind that spurious wakeups are possible to 30sec is just the maximum it will wait... you usually will need a while loop in this case. Also currently it seems to use ONE lock for ALL editors... meaning one (unrelated) editor can block others (e.g. two windows showing different source files).

I just want to outline how it would suppose to work in the best case:

There should be some kind of method to acquire the current state (from the code it seems one is interested actually in IJavaElement[]) as a completable future from the editor, this would then work like this:

synchronize on some lock object
return the current (maybe already completed) CF to the caller
if reconcilation is required, synchronize on the lock object, create a new CF (that will be completed one reconciliation is done with the expected result)

the codeminig (and probably others) would then fetch the CF and use an thenCompose(Async) (or similar) to transform the result (IJavaElement[]) to the final result (e.g. ICodeMinings[]) and return that.

That way one does not need an additional waiting thread or additional resource allocation and no need for basic synchronize primitives.

tobiasmelcher · 2025-12-16T16:36:10Z

@laeubi I’ve implemented the CompletableFuture-based changes you recommended with [3ab1bb4]. Could you please review the updated code when you have a chance?

laeubi

@tobiasmelcher thanks for your proposal I think it goes in the right direction already, I have added some comments below.

laeubi · 2025-12-17T06:02:57Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

 		final ISourceViewerExtension5 sourceViewer= fSourceViewer; // take a copy as this can be null-ed in the meantime
 		if (sourceViewer != null) {
-			reconciledViewers.add(sourceViewer);
+			CompletableFuture<CompilationUnit> future= toBeReconciledViewers.remove(sourceViewer);


Should it really be removed here?
You are calling updateCodeMinings in line 55 what then would need the future result?

I changed the sequence with [610a874] - now updateCodeMinings() ist first called and the future is therefore available.

It still feels wrong to remove it. What if no reconcilation happens but code minings are refreshed? Or the reconilation completes before we are asked to provide codminings?

What if no reconcilation happens but code minings are refreshed?

Such case where the trigger for code mining updates is "external" to document change is not to be handled by the reconciler then, but by some other, 3rd-party strategy that one could plug-in.

And IMO, unless it causes a problem, I think keeping the futures can be useful, for example to allow cancellation or chaining later on.

laeubi · 2025-12-17T06:04:06Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

 	public void aboutToBeReconciled() {
-		// interrupt code minings if modification occurs
-		reconciledViewers.remove(fSourceViewer);
+		toBeReconciledViewers.computeIfAbsent(fSourceViewer, unused->{return new CompletableFuture<CompilationUnit>();});


I would more would use compute here and then check if there is already one (and cancel it) and creating always a fresh one.

toBeReconciledViewers.compute(fSourceViewer, (viewer, existingFuture) -> { if (existingFuture != null) { existingFuture.cancel(false); } return new CompletableFuture<CompilationUnit>(); });

I tried with this code; but then it started again flickering. Reason: the canceled future now no longer returns the code minings and therefore the framework is removing the previous code minings provided by the Java implementation.
Maybe, I didn't get what you want to achieve. But I think that calling "cancel" will introduce the flickering again. Please convince me from the opposite if you think that calling "cancel" is a good idea.

laeubi · 2025-12-17T06:07:20Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+		CompletableFuture<CompilationUnit> future= toBeReconciledViewers.get(viewer);
+		return future;


You should use computeIfAbsent to make sure you always get one, even if I assume due to the code flow it should probably not be needed. This also should include checking if we already have some data (is that possible?) and then retrun an already completed future.

if "null" is returned then the source is reconciled and there is no need to wait for the future.

This is not a good design. If the source is reconciled it should here return an already completed future with the result, and the client should only ever call this method and there is no need to distinguish the different cases.

If the source is reconciled it should here return an already completed future with the result

I agree here that returning toBeReconciledViewers,computeIfAbsent(viewer, v -> unitFor(v)) would be cleaner and the impact on memory would be extremely low.

laeubi · 2025-12-17T06:11:19Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java


 	public static boolean isReconciled(ISourceViewerExtension5 viewer) {
-		return reconciledViewers.contains(viewer);
+		return !toBeReconciledViewers.containsKey(viewer);


This would also be a computeifabsent, then checking if the future was completed.

That way there is always one future holding the last state and we can check it.

The map contains only the viewers which should be reconciled. If reconciliation is done the entry will be removed from the map. If we keep the futures forever in the map, we need to find a time when to remove the entry from the map. This makes it more complex from my point of view. The current implementation of isReconciled returns the correct value. I don't see the need to change it. Please again convince me if you think that the current implementation is not correct.

If we keep the futures forever in the map, we need to find a time when to remove the entry from the map.

The time is actually when uninstalling the viewer.

Please again convince me if you think that the current implementation is not correct.

Se previous comments. The current logic is all prone to timing issues and as code mining (or even reconcilation!) can happen on different threads any time and might finish at random times.

Therefore there should only be two states:

The future is already completed because reconciliation has happened before

The future is currently pending because reconciliation is running.

and all this must be synchronized by the concurrent map.

The time is actually when uninstalling the viewer.

I agree with that.

laeubi · 2025-12-17T06:12:39Z

...t.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaElementCodeMiningProvider.java

+	}
+
+	private List<? extends ICodeMining> computeCodeMinings(ITextViewer viewer, IProgressMonitor monitor) {
+		monitor.isCanceled();


Even though in the original code, this line looks odd, what it its purpose?

Following monitor implementation is used for the code minings:

class CancellationExceptionMonitor extends NullProgressMonitor { @Override public boolean isCanceled() { boolean canceled= super.isCanceled(); if (canceled) { throw new CancellationException(); } return canceled; } }

I agree it looks odd. The code existed before, I have not invented it.

Alright this is really odd...

IIRC, the trick was that in order to properly honor cancellation inside a future, then the future must throw a CancellationException (so that it completesExceptionally and so on).
But maybe here it would be much cleaner to skip this particular CacnellationExceptionMonitor and inline the if (monitor.isCanceled()) throw new CancellationException(); iside the JavaElementCodeMiningProvider directly; or maybe have a method

void checkCancelled(IProgressMonitor monitor) throws CancellationException { if (monitor.isCancelled()) throw new CancellationException(); }

and replace all calls to monitor.isCancelled() by checkCancelled(monitor)

laeubi · 2025-12-17T06:14:29Z

...t.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaElementCodeMiningProvider.java

+			CompletableFuture<CompilationUnit> future= JavaCodeMiningReconciler.getFuture(v);
+			if (future != null) {
+				return future.thenApplyAsync(ast -> {
+					return computeCodeMinings(viewer,monitor);


Why is the AST unused here?
It feels we should call collectMinings directly here

done with [610a874]

laeubi · 2025-12-17T06:18:23Z

...t.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaElementCodeMiningProvider.java

+			monitor.isCanceled();
+			return minings;
+		} catch (JavaModelException e) {
+			// Should never occur


The usual pattern would be something like

future = new CompletableFuture<> //use whatever Threadpool you think sufficient e.g the commonPool execute(()-> { try { //compute your possible throwing operations here // if progress monitor is cancel cancel and exit early future.complete(...) } catch (Exception) { future.completeExceptionally(...) } } return future;

Is this really needed? Please give me more details and the concrete code how it should look like when JavaCodeMiningReconciler.getFuture() returns null.
I think that the current implementation fixes the original problem and works fine.

This is not strictly required I think I just saw the TODO here...

laeubi

See some comments below

laeubi · 2025-12-18T05:32:34Z

...t.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaElementCodeMiningProvider.java

+	}
+
+	private List<? extends ICodeMining> computeCodeMinings(ITextViewer viewer, IProgressMonitor monitor) {
+		monitor.isCanceled();


Alright this is really odd...

laeubi · 2025-12-18T05:33:19Z

...t.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaElementCodeMiningProvider.java

+			monitor.isCanceled();
+			return minings;
+		} catch (JavaModelException e) {
+			// Should never occur


This is not strictly required I think I just saw the TODO here...

laeubi · 2025-12-18T05:41:08Z

...org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaMethodParameterCodeMiningProvider.java

+			collectLineContentCodeMinings(unit, minings);
+			if (viewer instanceof ISourceViewerExtension5) {
+				ISourceViewerExtension5 codeMiningViewer= (ISourceViewerExtension5)viewer;
+				if (!JavaCodeMiningReconciler.isReconciled(codeMiningViewer)) {


This check looks odd to me... we already have the unit, we have calculated the codeminigs and now we are suddenly not able anymore?

This then would just cancel the future effectively what we previously discovered seems not working well....?

@jjohnstn any idea whats the purpose of this? For me this all looks highly questionable and prone to timing issues, the document might not be reconciled just right after this method returns, it might get (randomly) already reconciled before we complete here and so on...

laeubi · 2025-12-18T05:42:29Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

 		final ISourceViewerExtension5 sourceViewer= fSourceViewer; // take a copy as this can be null-ed in the meantime
 		if (sourceViewer != null) {
-			reconciledViewers.add(sourceViewer);
+			CompletableFuture<CompilationUnit> future= toBeReconciledViewers.remove(sourceViewer);


It still feels wrong to remove it. What if no reconcilation happens but code minings are refreshed? Or the reconilation completes before we are asked to provide codminings?

laeubi · 2025-12-18T05:44:08Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+		CompletableFuture<CompilationUnit> future= toBeReconciledViewers.get(viewer);
+		return future;


This is not a good design. If the source is reconciled it should here return an already completed future with the result, and the client should only ever call this method and there is no need to distinguish the different cases.

laeubi · 2025-12-18T05:48:45Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java


 	public static boolean isReconciled(ISourceViewerExtension5 viewer) {
-		return reconciledViewers.contains(viewer);
+		return !toBeReconciledViewers.containsKey(viewer);


If we keep the futures forever in the map, we need to find a time when to remove the entry from the map.

The time is actually when uninstalling the viewer.

Please again convince me if you think that the current implementation is not correct.

Se previous comments. The current logic is all prone to timing issues and as code mining (or even reconcilation!) can happen on different threads any time and might finish at random times.

Therefore there should only be two states:

The future is already completed because reconciliation has happened before

The future is currently pending because reconciliation is running.

and all this must be synchronized by the concurrent map.

tobiasmelcher · 2025-12-18T16:04:49Z

Thank you so much for taking the time to review my work and share your feedback; I really appreciate your effort and support.
I believe [8143972] addresses the changes you suggested. To make sure I can learn and improve more effectively, it would be extremely helpful if future feedback could include more concrete examples or pointers to the exact code changes. For instance, highlighting something like:

.handle((result, ex) -> {
    if (ex instanceof CompletionException ce &&
        ce.getCause() instanceof CancellationException) {
        monitor.setCanceled(true);
    }
    return result;
}

in JavaElementCodeMiningProvider, and explaining why monitor.setCanceled(true) needs to be called so that the CodeMiningFramework does not delete previous code minings from the Java provider and flickering occurs.
Maybe you already tried to convey this, but I didn’t fully understand it at first. Your guidance would make the process much easier and help me deliver better results.
Thanks again for your collaboration!

laeubi

Thanks for the improvements I think its overall going in a good direction, I added two more comments here.

laeubi · 2025-12-18T16:18:00Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+		final ISourceViewerExtension5 sourceViewer= fSourceViewer;
+		if (editor != null && sourceViewer != null) {
 			sourceViewer.updateCodeMinings();
+			// Use remove() to ensure atomicity


I'm not sure why you are so eager here to remove the future, I also don't understand why it is needed for atomicity?

Currently, whenever a reconcilation is request, the old feature is canceld and a new one is generated and this is one atomic operation in the map, so consumers will see a consistent state.

Then hopefully at some point in time the reconcilation is done and that feature is completed, so why then remove it?

// CURRENT CODE - PROBLEMATIC CompletableFuture<ITypeRoot> future = typeRootFutureByViewer.get(sourceViewer); if (future != null && ast.getTypeRoot() != null) { future.complete(ast.getTypeRoot()); // ❌ Race: future might be replaced after get() }

Problem: Between get() and complete(), another thread could call aboutToBeReconciled(), which cancels and replaces the future. You’d then complete a cancelled/stale future.

Is this an AI suggestion or something you actually observe? The suggestion here makes no sense of course the future might be canceled but this is on purpose and our computation is going vanished and a new cycle then starts and calls (again) this method where we complete the then current one...

I could somehow follow the explanation. If it makes no sense, I will then replace the remove call.

laeubi · 2025-12-18T16:19:07Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java


-	public static boolean isReconciled(ISourceViewerExtension5 viewer) {
-		return reconciledViewers.contains(viewer);
+	public static boolean isReconciled(ITextEditor editor) {


I think we should be able to remove this method entierly, it should never bee needed as we use the CF anyways.

coordination Problem: Previous implementation returned emptyList() immediately when reconciliation wasn't complete, causing code minings to disappear/reappear during AST reconciliation → UI flickering Solution: - Add shared RECONCILE_LOCK and reconciledViewers Set for coordination - Code mining provider now *waits* (up to 30s) for reconciliation to complete - Reconciler signals completion with notifyAll() in reconciled() - Remove viewer from reconciled set in aboutToBeReconciled() during edits

laeubi

It looks almost good to me, just some more comments of some details from my side.

laeubi · 2025-12-20T06:18:11Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+		if (ast == null) {
+			return;
+		}


This check should be included in the future completion part as otherwise the future is maybe never completed.

laeubi · 2025-12-20T06:20:12Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+			if (future != null && !future.isDone() && ast.getTypeRoot() != null) {
+				future.complete(ast.getTypeRoot());
+			}


I think this needs to be split up in two parts, maybe like this:

Suggested change

if (future != null && !future.isDone() && ast.getTypeRoot() != null) {

future.complete(ast.getTypeRoot());

}

if (future != null && !future.isDone()) {

if(ast !=null && ast.getTypeRoot() != null) {

future.complete(ast.getTypeRoot());

} else {

future.cancel(false);

}

}

but I'm not sure in wich cases the AST (or typeroot) can be null, so maybe one should then better complete it with a suitable Exception message instead?

laeubi · 2025-12-20T06:21:10Z

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/JavaCodeMiningReconciler.java

+		ITypeRoot unit= EditorUtility.getEditorInputJavaElement(editor, true);
+		CompletableFuture<ITypeRoot> future= new CompletableFuture<>();
+		future.complete(unit);
+		return future;


As you check for null above, should you check here for null as well?

laeubi · 2025-12-20T06:22:45Z

...org/eclipse/jdt/internal/ui/javaeditor/codemining/JavaMethodParameterCodeMiningProvider.java

-				return CompletableFuture.completedFuture(Collections.emptyList());
+		ITextEditor textEditor= super.getAdapter(ITextEditor.class);
+		CompletableFuture<ITypeRoot> future= JavaCodeMiningReconciler.getFuture(textEditor);
+		return future.thenApplyAsync(typeRoot -> {


You might want to use orTimeout here just in case so we do not block forever.

Ref: eclipse-jdt#2698 (review)

This was referenced Dec 15, 2025

CodeMining: reuse existing inlined annotations when minings are unchanged eclipse-platform/eclipse.platform.ui#3580

Open

Preserve last code minings for non-reconciled viewers and clean up on viewer disposal #2697

Closed

jjohnstn reviewed Dec 15, 2025

View reviewed changes

laeubi reviewed Dec 17, 2025

View reviewed changes

laeubi reviewed Dec 18, 2025

View reviewed changes

tobiasmelcher force-pushed the wait_for_reconciler branch from 73fe5ab to 52a105a Compare December 18, 2025 18:10

tobias-melcher added 6 commits December 18, 2025 19:11

use CompletableFuture instead of Object.wait

9817a0c

add changes requested by review

98a8768

Use ITextEditor as key and ITypeRoot futures in JavaCodeMiningReconciler

99d905c

fix test compile errors

0b2f04c

remove isReconciled

b7da8a6

tobiasmelcher force-pushed the wait_for_reconciler branch from 52a105a to b7da8a6 Compare December 18, 2025 18:12

laeubi reviewed Dec 20, 2025

View reviewed changes

add null check for AST and orTimeout call after future.thenApplyAsync

8246b94

Ref: eclipse-jdt#2698 (review)

jchamlin mentioned this pull request Dec 28, 2025

Eclipse Code mining: Line jumping flickering with Copilot installed although completion is disabled microsoft/copilot-eclipse-feedback#87

Open

		CompletableFuture<CompilationUnit> future= toBeReconciledViewers.get(viewer);
		return future;

-			if (future != null && !future.isDone() && ast.getTypeRoot() != null) {
-				future.complete(ast.getTypeRoot());
-			}
+			if (future != null && !future.isDone()) {
+			    if(ast !=null && ast.getTypeRoot() != null) {
+       				future.complete(ast.getTypeRoot());
+				} else {
+						future.cancel(false);
+				}
+			}

Fix UI flickering in code mining by adding wait/notify reconciliation coordination #2698

Are you sure you want to change the base?

Fix UI flickering in code mining by adding wait/notify reconciliation coordination #2698

Conversation

tobiasmelcher commented Dec 15, 2025

Uh oh!

jjohnstn left a comment

Choose a reason for hiding this comment

Uh oh!

mickaelistria commented Dec 16, 2025

Uh oh!

laeubi commented Dec 16, 2025

Uh oh!

tobiasmelcher commented Dec 16, 2025

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment