Problem during restore of ClassFileEditor#1876
Problem during restore of ClassFileEditor#1876mehmet-karaman wants to merge 1 commit intoeclipse-pde:masterfrom
Conversation
|
@mehmet-karaman I really doubt this is the "right" thing to came up with something new here, can you please explain why we need a new interface here? |
Test Results 6 files - 759 6 suites - 759 22s ⏱️ - 55m 16s Results for commit 750c681. ± Comparison against base commit 4d59605. This pull request removes 3607 tests. |
Because somebody merged #1552... But seriously: the problem is that JDT UI after #1552 needs to know whether classpath containers, that were always fully initialized by JDT core before, now sometimes don't do anything on init and so JDT core provides basically "empty" classpaths for PDE based projects. |
It only does when callers ask for classpath while the Jobframework is offline what can lead to infinite blocking of the UI, but sure its easier to "blame" someone else PR instead of fixing this really bad behavior. Beside that, a classpath can change anytime but some code seem not to account for that fact, what immediately would fix the problem as well because then you would only see an empty editor for the time until the classpath is actually updated. |
Which is the root cause for eclipse-jdt/eclipse.jdt.ui#2245
Which was like this since ~20 years and no one complained...
The fix is right here, and I would expect the fix would be provided by the same person who introduced regression. |
Yes and 20 years before people where using 32 bit CPUs and 56k Modems and have their target platforms local on the hard disk... Times are changing and now we have Clouds and Servers that not always respond in time and Firewalls that block request. And that no one complained is simply wrong see here: also people are getting more used to kill their Eclipse instead to bother reporting a problem that gets closed because its not easy to reproduce.
I can tell that PDE will not accept that "fix" as it introduces the same bad behavior as before just at another call-path. I have documented the problem here:
so if someone is eager to work on that please do so, unless this is fixed it is unlikely the situation will improve. Just because it is wrong for 20 years (what might or might not be the case as no one has checked since JDT is calling that code from UI during startup) does not mean it is acceptable to be wrong for the next 20 years. |
Let me guess: you haven't tried the patch at all? |
You are not PDE. |
|
This exchange is going in a poor direction. Please keep in mind that all of us want eclipse to be excellent and each of us invests a lot of time toward that goal. Opinions will sometimes differ on the best course of action. We can all agree there is some very unfortunate timing behavior on startup. It would seem best to address this without going back to doing long running network/disk io on the UI thread on startup. I suppose one might argue that is just my opinion which indeed it is. |
|
Just for the matter of fact I would expect that an Also from a technical point of view, if Beside that, the problem it is claiming to solve can be reproduced without PDE involved: |
This is a different problem : eclipse-jdt/eclipse.jdt.core#4208 (comment) If you are confused by "async", let's name it "lazy". That would surely better match PDE container behavior. |
The classpath container is neither lazy nor async and the whole build process and the whole PDE tooling can work with it without a problem. Also this "new" API does nothing else than calling the same API that is already available, so if you think it is the right fix, then simply call While I strongly believe that this will open the door for many concurrency issues and only shows that code is called before things are properly initialized at some other place, it seems the "right" thing to do here instead of putting the burden for such hacks to other projects. |
Well, except that now we have regression eclipse-jdt/eclipse.jdt.ui#2245. This regression is caused by the hack in PDE, and now we providing a way to fix the regression via API, making the hack not a hack anymore. |
|
I’m really not sure how to judge this change without an overview of where and how this is used. How does this resolve problems? At this point it doesn’t even compile. I suppose one question is what happens in the caller when isInitialized returns false? Perhaps we can reserve judgment until a complete picture has been sketched… |
All valid points. I apologize the PR's were created without proper explanation. This PR is one from three PR's needed to fix regression introduced by #1552, see @mehmet-karaman will provide an overview over the problem we fix : steps to reproduce, reason why #1552 caused that and how the proposed patches are supposed to fix this particular regression. |
|
Hello, The problem can easily reproduced with the following steps:
After the restart you will see that the ClassFileEditor coulnd't be restored as before the changes in PDE. My investigations on that side was that the PDE classpath container wasn't resolved before the editor was restored. From jdt.ui or jdt.core side I don't have access to PDE so I don't know whether the PDE classpath container is initialized or not. So I had to introduce a new classpath container interface which is able to tell me if its initialized or not. @laeubi I guess this is the answer for your question why there is a new Interface introduced. On JDT side i have implemented a delayed IEditorInput which delays any UI action until the PDE classpath container is initialized. After the PDE classpath container is initialized I retrieve the original IEditorPart and execute the delayed UI executions. This solves the problem. @iloveeclipse sorry that you got confronted with these critical comments. For all others your questions and reviews all welcome. Feel free to ask further questions I am willing to answer them. Thank you in advance. Best regards |
No apologies are needed. I know everyone has the best intentions. It seems we have developed some sensitivities on this topic which is also fully understandable given how frustrating and intractable the problem seems to be. Let’s keep an open mind and look forward. We’re one big team. |
- Changed the RequiredPluginsClasspathContainer interface from IClasspathContainer to IAsyncClasspathContainer See eclipse-jdt/eclipse.jdt.ui#2245
|
See comment here: and supporting PR:
nothing of that requires new API... if the classfile is not on the classfile it does not means the container is "lazy" it does means the initialization is in progress (in a background thread). In the worst case this simply fetches all content twice. |
This is incorrect or at least incomplete description. At the time where the EditorInput(!) is restored from the memento, the class file can happily be restored (see eclipse-jdt/eclipse.jdt.ui#2349 ), but then there is a check if the type is found by the project and if not
At this point there actually is not classpath container, and containers never get initialized by themselfs, JDT do initialize them what again can change anytime when one updates the classpath of a project (e.g. add a new required bundle).
This is not required, JDT itself controls the init of classpath container whenever one calls |
|
I have now came up with a PR that describes the proposed changes in the editor without any new API and without any adjustment to PDE or classpath containers at all: I was able to restore a |
|
I have now submitted a fix for the ClassFileEditor editor here: as it does not require any changes to API or PDE at all I would assume this PR obsolete now, and therefore closing it. If later something more is needed we can reopen and/or create a new one. |
4 participants
eclipse-jdt/eclipse.jdt.ui#2347