Do not wait for AST when copying code in ClipboarOperationAction

[fedejeanne]

What it does

Does not let the call to SharedASTProviderCore.getAST(...) in ClipboarOperation wait for the AST of the class. This keeps the call from landing in this line...

eclipse.jdt.ui/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/core/manipulation/CoreASTProvider.java

Line 176 in 3a6d922

fWaitLock.wait(30000); // XXX: The 30 seconds timeout is an attempt to at least avoid a deadlock. See https://bugs.eclipse.org/366048#c21

... which is known to cause UI freezes when the call is done in the UI thread (which is the case here, #2028 ).

TL;DR
Fixes #2028 without causing the leak reported in #2403 (I triggered the tests locally by adding the Extra VM Arguments mentioned here)

How to test

Copy code from one class to the other (you may use the classes provided in #2382 (comment)).
If the AST was present at the moment of the copy operation then the imports will be present in the target class. If the AST was not ready at the time of the copy operation then the imports will not be copied in the target class.

Disclaimer
This PR aims to avoid the UI freeze and it does so at the cost of maybe (just maybe) not copying the imports when copying code. This is a small tradeoff that I consider acceptable since UI freezes are IMO the absolute worst. I plan to come back to this complex issue (the "deadlock" that happens when waiting in the UI freeze) but for the moment, this PR does the absolute minimum change possible that gets rid of the UI freeze.

It would be nice to get it merged for the next release so (sorry for the spam but) I'll take the liberty of asking for your input directly @jjohnstn , @iloveeclipse and @laeubi .

Author checklist

• I have thoroughly tested my changes, but manually and by triggering the JavaLeakTest locally
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[fedejeanne]

All test failures are unrelated since none of the failing tests even runs into the code that this PR modifies.

[fedejeanne]

Any chances of getting this merged by EOD today?

This change is pretty safe and it does prevent UI freezes.

Other methods already take the same approach, .e.g CleanUpPostSaveListener::getAst

eclipse.jdt.ui/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/CleanUpPostSaveListener.java

Line 589 in ef74c44

CompilationUnit ast= SharedASTProviderCore.getAST(unit, SharedASTProviderCore.WAIT_NO, monitor);

[jjohnstn]

Any chances of getting this merged by EOD today?

This change is pretty safe and it does prevent UI freezes.

Other methods already take the same approach, .e.g CleanUpPostSaveListener::getAst

eclipse.jdt.ui/org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/CleanUpPostSaveListener.java

Line 589 in ef74c44

CompilationUnit ast= SharedASTProviderCore.getAST(unit, SharedASTProviderCore.WAIT_NO, monitor);

I'm not sure that is an entirely accurate statement. In the code cited above, it indeed doesn't wait on the getAST call and sees if the AST is available but if it isn't, it parses the unit and creates an AST. Could that be done in this scenario as well perhaps in a non-UI thread?

[fedejeanne]

You're right, I picked a bad example because CleanupPostSaveListener::createAst has a fallback mechanism. And yes, that can be done in another thread (see my other PR #2415 ), but this is beside the point.

The point I wanted to make is that the code is ready to handle null as the result of calling SharedASTProviderCore.getAST(...). In the case of this PR, getting null back would mean that the imports didn't get copied to the clipboard and therefore won't be added to the class where you paste the code (you then probably have to manually run "Organize imports" to get them).

Perhaps a better example of not waiting for the AST is in org.eclipse.jdt.internal.ui.text.java.hover.JavaSourceHover.getBracketHoverInfo(ITextViewer, IRegion):

eclipse.jdt.ui/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/java/hover/JavaSourceHover.java

Lines 504 to 506 in ef74c44

	CompilationUnit ast= SharedASTProviderCore.getAST(editorInput, SharedASTProviderCore.WAIT_NO, null);
	if (ast == null)
	return null;

In such case, if you get null back you simply do not display information about the beginning of a block when you hover over a closing bracket (this popup would be missing).

Sorry for the confusion.

[jjohnstn]

@fedejeanne I'm ok with this temporary fix. I'll wait a bit to see if there any objections from @iloveeclipse or @laeubi.

[fedejeanne]

@fedejeanne I'm ok with this temporary fix. I'll wait a bit to see if there any objections from @iloveeclipse or @laeubi.

Thank you, much appreciated :-)

[fedejeanne]

@fedejeanne I'm ok with this temporary fix. I'll wait a bit to see if there any objections from @iloveeclipse or @laeubi.

@laeubi , @iloveeclipse , any objections?

[iloveeclipse]

Should the operation be differently executed if we know it might freeze UI?
With this change users will see unpredictable outcome of same action with same input.

Also I miss anylysis of original issue, just not waiting for AST means something is blocking if we wait, and if that blocks one operation it might block others, so if we continue to switch off functionality without understanding the root cause we may end with different features not working as designed...

I'm out of PC till end of August, so I can't analyse the issue in depth.

[fedejeanne]

Should the operation be differently executed if we know it might freeze UI?

What do you have in mind? My last attempt to do it differently (#2382) caused a leak (#2403).

With this change users will see unpredictable outcome of same action with same input.

For the most part, the behavior will remain the same. Only when some other editor has requested an expensive AST that is still "blocking" the CoreASTProvider::getAST may (only may) the user experience a different behavior. All of the following needs to hold true for the user to experience a different behavior:

• Some editor has requested an expensive AST
• That AST is still being calculated
• The lines copied contain references to classes that are not present in the class where the text is going to be copied

If all of that holds true then the user will paste the text in the target editor and he'll be missing some imports. He then needs to press Ctrl + Shift + o and that's it.

This is a tradeoff that would probably wouldn't happen very often and the result is bearable, way better than an UI freeze for an indefinite amount of time.

Also I miss anylysis of original issue, just not waiting for AST means something is blocking if we wait, and if that blocks one operation it might block others, so if we continue to switch off functionality without understanding the root cause we may end with different features not working as designed...

This #2028 (comment) is as far as we know about this issue. Shortly summarized: the method is oddly written and the class uses old-fashioned wait and notify mechanisms which make it really difficult to see who "forgot" to wake up other threads waiting for the AST (which is why one lands in this infamous line):

eclipse.jdt.ui/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/core/manipulation/CoreASTProvider.java

Line 176 in 78dac5b

fWaitLock.wait(30000); // XXX: The 30 seconds timeout is an attempt to at least avoid a deadlock. See https://bugs.eclipse.org/366048#c21

Fixing the whole class is not something that can be done easily and there have been failed attempts already.

I know that this PR doesn't fix the underlying issue (expensive, non-cancelable AST requests made in the UI thread), but that is something that would sadly need more time to be fixed and in the meantime, UI freezes are happening and they really degrade the UX. To the point where our developers sometimes even kill the IDE process and choose to loose all their work instead of waiting for the UI to unfreeze. They end up being frustrated with the IDE and peg it (once again) to the well-known stigma: "Eclipse is slow".

All in all: I'd rather buy us some time until we can properly analyze and fix the AST calculation in CoreASTProvider. This PR does just that at a very low price.

[fedejeanne]

@iloveeclipse I found the culprit: Semantic Highlighting Reconciler. See my #2028 (comment) . The important part regarding the degradation of the functionality (imports not being copied) is that the imports would be copied once the AST has been calculated.

This would mean that the copy operation would only get the imports (should there be any necessary) iff the AST for the currently opened editor has already been calculated.

In other words: even after merging this PR, the loss of functionality (missing imports) would only happen while the AST is being calculated. Imaging that the currently opened editor contains a huge class and it takes the highlighting reconciler 2 minutes to get the AST, any copy operation before those 2 minutes have elapsed would have no imports (functionality degradation) but every copy operation after the 2 minutes have elapsed would have the imports (no degradation).

I hope this level of detail in the analysis is good enough and the tradeoff is acceptable for you. Let me know if there are any questions :-)

Looking forward to your answer.

[iloveeclipse]

In other words: even after merging this PR, the loss of functionality (missing imports) would only happen while the AST is being calculated. Imaging that the currently opened editor contains a huge class and it takes the highlighting reconciler 2 minutes to get the AST, any copy operation before those 2 minutes have elapsed would have no imports (functionality degradation) but every copy operation after the 2 minutes have elapsed would have the imports (no degradation).

Sounds reasonable.
Please rebase on master before merge. I see 21 test fails, this doesn't look nice.

[fedejeanne]

I've rebased. The test failures are unrelated though since they don't run into the code I changed in this PR, I tested locally (see #2406 (comment)).

[fedejeanne]

@iloveeclipse rebase done. Same result.

[iloveeclipse]

@iloveeclipse rebase done. Same result.

Could it be, you run into #2423?
Please double check, if the fails are different, please open new ticket.

[jjohnstn]

@iloveeclipse rebase done. Same result.

Could it be, you run into #2423? Please double check, if the fails are different, please open new ticket.

These are the same 21 failures that have plagued our Jenkin's runs for a while now. It is not due to this change.

[fedejeanne]

Thank you @jjohnstn ! That means that this PR is ready to be merged, right on time for the upcoming release 👏

[laeubi]

Fixing the whole class is not something that can be done easily and there have been failed attempts already.

I would not say it really fails, from my side it more lacked interest of the JDT team to improve that part of the code as it seem to cry "don't touch me". So if someone form the JDT team is willing to review code changes there I would update/recreate the PR.

In general it feels that callers of that method maybe better want a (cancelable) way to being notified when the AST is ready than a (blocking) approach. e.g. for the copy clipboards action it might be fine to insert the text immediately and add the missing imports later (hopefully only a few milliseconds).