Can not save dirty .java editor, when opened programmatically

[LorenzoBettini]

I know that https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1051 has been fixed and with the latest i-build, org.eclipse.jdt.ui_3.32.0.v20240122-1112.jar, I cannot reproduce that bug anymore in my development environment.

However, we still found something similar in some of our Xtext UI tests (https://github.com/eclipse/xtext/issues/2895), even with the latest i-build in the target platform (the JDT UI is the same as above). In those tests, we programmatically open Java editors, do some changes and its contents and the editor correctly becomes dirty; then, we programmatically save a dirty editor with org.eclipse.ui.IWorkbenchPage.saveEditor(IEditorPart, boolean), passing false. The call succeeds but the editor stays open (and dirty). This happens rarely, but it happens:

In the screenshot above, the "Foo.java" editor was saved, while the "Bar.java" editor was not, although the programmatic call succeeded also for that editor.

Trying to save the dirty editor when that UI test fails, i.e., interacting with the running test workbench, still does not save it.

So I wondered if the fix of #1051 has effectively reverted all the problematic code or if something related remained.

As a final note, #1051 says "Even After "Save" or "Save All" .java Editor keeps showing a '*' in front of the Name" but the problem was much worse. It was not only the editor was still dirty: the file contents were not saved at all. Even if you closed the dirty editor and accepted the dialog proposal to save the file, the file was not saved at all.

[iloveeclipse]

@jukzi : could you please check?

[jukzi]

@LorenzoBettini i need more info to reproduce it within platform. A Junit would be nice. You could also please try yourself which commit broke it - or if it is related to callReadOnly by replacing org.eclipse.jdt.internal.core.JavaModelManager.callReadOnlyUnchecked(JavaCallable<T, E>) with

    private <T, E extends Exception> T callReadOnlyUnchecked(JavaCallable<T, E> callable) throws E {
        return callable.call();
    }

[LorenzoBettini]

@jukzi as I said, it is not systematically reproducible even in our UI tests, so it's hard to provide a test for that.

I can try what you suggest. If I understand correctly, I should have in my workspace jdt.core and try to change that call right?

[jukzi]

I can try what you suggest. If I understand correctly, I should have in my workspace jdt.core and try to change that call right?

yes, and then launch your Junit or a debugee so that the change is used

[LorenzoBettini]

@jukzi So I did what follows:

imported jdt.core sources in my workspace

changed this method (org.eclipse.jdt.internal.core.JavaModelManager.callReadOnlyUnchecked(JavaCallable<T, E>)):

    private <T, E extends Exception> T callReadOnlyUnchecked(JavaCallable<T, E> callable) throws E {
        boolean hadTemporaryCache = hasTemporaryCache();
        try {
            getTemporaryCache();

            return cacheZipFiles(callable);
        } finally {
            if (!hadTemporaryCache) {
                resetTemporaryCache();
            }
        }
    }

into this one

    private <T, E extends Exception> T callReadOnlyUnchecked(JavaCallable<T, E> callable) throws E {
        return callable.call();
    }

With the debugger I made sure this source code is effectively used.

I then ran our UI tests over and over again. I made 6 suite executions. Each suite execution runs the same tests over and over again about 100 times. I did not see the problem anymore! (previously, after about 50 executed tests the problem showed up).

So I'd be tempted to say that the culprit was exactly that method :)

[jukzi]

please give stacktrace(s) when that method is called and error occured. i.e. add new RuntimeException().printStackTrace(); reproduce the error and post the result.

[LorenzoBettini]

Sorry, I'm not sure I understand, could you please be more specific?

I should revert to the original method in jdt.core and then put the new RuntimeException().printStackTrace(); where?

[jukzi]

I should revert to the original method in jdt.core

yea put it at the start of callReadOnlyUnchecked

[LorenzoBettini]

@jukzi I tried and I don't think it's gonna work: everything is so slowed down because of the print of the exception that the tests take ages to execute and makes Eclipse unresponsive.

I only managed to take this screenshot (I couldn't select anything from Eclipse because it was locked), but I'm not sure this represents a situation where the problem would have occurred or simply everything hanged:

[jukzi]

@LorenzoBettini that stacktrace looks like https://github.com/eclipse-jdt/eclipse.jdt.ui/pull/1119 would repair it, please try that

[LorenzoBettini]

@jukzi I've just tried that (I removed jdt.core from workspace, imported jdt.ui and applied the patch, made sure it is executed) but the problem comes back again :'(

So, only the changed of https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1118#issuecomment-1905800344 solved the problem

[jukzi]

@LorenzoBettini i will need the offending stacktrace - it should be possible to log the stacktraces - just only enable output short before you try to save the file. (use a trigger-breakpoint point + printStackTrace in another breakpoint) Or you could try to disable readonly-cache one by one - there are not many possibilites that are actually used in you scenario:

[LorenzoBettini]

@jukzi I don't understand the first part. If I put the new RuntimeException().printStackTrace before saving, would that be helpful even when the problem does not occur?

[jukzi]

i mean if "new RuntimeException().printStackTrace();" slows you down too much, you can put it in a break point condition and have that breakpoint disabled until you need it.

Breakpoints can even be automatically disabled until another Triggerpoint is reached:

[LorenzoBettini]

But if the programmatic call to save always succeeds, even when the editor is not closed, how can I enable that breakpoint? In any case, the call to save has already returned.

concerning this part, which seems to be more feasible

Or you could try to disable readonly-cache one by one - there are not many possibilites that are actually used in you scenario:

what should I disable one by one?

[jukzi]

instead of using "JavaCore.callReadOnly" / "JavaCore.runReadOnly" in jdt.ui you could call() / run() only the argument and see which use of readOnly fixes your issue.

[LorenzoBettini]

@jukzi maybe it's early to celebrate but with these two commits together I cannot seem to reproduce the problem anymore https://github.com/LorenzoBettini/eclipse.jdt.ui/commit/fb4934f08520c18c84324cf157f1e8cacda1bdd3 https://github.com/LorenzoBettini/eclipse.jdt.ui/commit/11c4f69ebf7723f0746dd8656e19ebf1c89778b8 does it make sense to you?

[iloveeclipse]

Hmm, https://github.com/LorenzoBettini/eclipse.jdt.ui/commit/fb4934f08520c18c84324cf157f1e8cacda1bdd3 alome is not enough?

[LorenzoBettini]

Hmm, LorenzoBettini@fb4934f alome is not enough?

No, I confirm: it is NOT enough (that's the first experiment I made, because it looked like a good candidate); just like the other one alone is NOT enough. They are both needed.

[jukzi]

can you please post the stacktrace of the offending computeAdornmentFlags? i really wonder how that can be an issue

[LorenzoBettini]

OK, let's see if I understand correctly. I placed my jdt.ui on current master (so, not with my commits), https://github.com/eclipse-jdt/eclipse.jdt.ui/commit/6a8301e7125a7b1a3e93294ea9e8601b006c6477.

I put a new RuntimeException().printStackTrace(); at the beginning of org.eclipse.jdt.ui.OverrideIndicatorLabelDecorator.computeAdornmentFlags(Object).

I run my tests and this is what I see (NOTE: this is the printed stack trace on a normal run, NOT when the problem occurs, as I said, it's hard to do that in that occasion):

java.lang.RuntimeException
    at org.eclipse.jdt.ui.OverrideIndicatorLabelDecorator.computeAdornmentFlags(OverrideIndicatorLabelDecorator.java:129)
    at org.eclipse.jdt.ui.OverrideIndicatorLabelDecorator.decorate(OverrideIndicatorLabelDecorator.java:267)
    at org.eclipse.ui.internal.decorators.LightweightDecoratorDefinition.decorate(LightweightDecoratorDefinition.java:254)
    at org.eclipse.ui.internal.decorators.LightweightDecoratorManager$LightweightRunnable.run(LightweightDecoratorManager.java:105)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
    at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.decorate(LightweightDecoratorManager.java:359)
    at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.getDecorations(LightweightDecoratorManager.java:345)
    at org.eclipse.ui.internal.decorators.DecorationScheduler$1.queue(DecorationScheduler.java:410)
    at org.eclipse.ui.internal.decorators.DecorationScheduler$1.run(DecorationScheduler.java:388)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

If I understand correctly, that takes place asynchronously, in fact, there's nothing from our tests.

I pasted only one, since the other ones look the same.

Is that what you asked?

[jukzi]

@LorenzoBettini please try this: https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1914

[LorenzoBettini]

@jukzi I've just tests your branch (to be clear, I have NOT used my commits on jdt.ui, only imported jdt.core after cloning your branch from your fork); tested it on Linux and macOS and the problem is GONE! :)

Just for curiosity, how is the above stack trace on that method from jdt.ui related?

[jukzi]

Just for curiosity, how is the above stack trace on that method from jdt.ui related?

i don't know. i just gave up using that cache, because i could not figure out what exactly gone wrong.

[LorenzoBettini]

@jukzi will https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1914 be merged?