WorkspaceFolders: Fix interpretation of event flags #116

ahmedneilhussain · 2022-05-06T10:46:22Z

LSP4e implements the LSP Workspace Folders protocol [if supported by the remote back end] using the Eclipse ResourceListener mechanism. Looking at the current implementation and reading the Eclipse source code, it isn't using the eclipse API quite correctly.

The flags integer for an IResourceDelta seems to be a synthesis of an enum "Kind" (bottom 8 bits) and a bit mask (top 24 bits). The existing LSP4e code tries to detect a project open/close with delta.getKind() == IResourceDelta.OPEN && project.isAccessible().
That won't work, because OPEN is a qualifying bitmask flag, not a kind - OPEN = 0x4000 but getKind() returns flags & 0xFF.
The kind for an open/close event is a CHANGE, but with the OPEN flag set.

We also add PRE_DELETE to the filter of ResourceListener events we are observing: this gives a cleaner behaviour on hard deletes if the underlying language server is also observing the file system, as sometimes delete events are only sent when the file tree is already gone.

…ectly. Avoid sending events with null content.

Tweak mock server initialisation so that the capabilities can be customised Move the logic for unsubscribing from Eclipse resource events a bit higher up LanguageServerWrapper.stop() to avoid potential race condition and NPE

mickaelistria

Thanks for this good patch! To conform with Eclipse license and IP requirements, please sign the ECA (link is on the review part of this PR) and add copyright header to new files.

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/WorkspaceFoldersTest.java

mickaelistria · 2022-05-09T09:22:58Z

~~There is now 1 failing test (regression) that is related to that change, see https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-116/2/display/redirect~~ Seems like bad luck, not reproduced.

rubenporras · 2022-05-09T10:39:54Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

+			} else {
+				return null;
+			}
+		} else if (e.getType() != IResourceChangeEvent.POST_CHANGE) {


could we do this at the very beginning of the method, so that if this condition is met, we do not need to create an WorkspaceFoldersChangeEvent that we will never use?

Sure, done. NB I kept this guard (and enhanced it for the extra event type I've added) but if the subscription at line 499 is honoured by Eclipse it should not to be needed, is that correct? I'm a relative newcomer to programming on the Eclipse platform so I'm never totally sure how defensive/paranoid to be...

I am also a relative newcomer to programming on the Eclipse platform. Normally I would code as by API and remove the guard, since it should never be hit. According to git history @mickaelistria introduced the guard, so maybe he has some insight why he added it in the first place.

if the subscription at line 499 is honoured by Eclipse it should not to be needed, is that correct?

Correct. However, it's always safer to re-validate important assumptions in the code. Adding a check has no significant impact on the performance but has a very positive impact on our capacity to change the code safely, so overall it's beneficial.

Thanks. Noted for future!

rubenporras · 2022-05-09T10:43:11Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

+					if ((delta.getKind() == IResourceDelta.ADDED || (delta.getKind() == IResourceDelta.CHANGED
+							&& (delta.getFlags() & IResourceDelta.OPEN) == IResourceDelta.OPEN))
+							&& project.isAccessible()
+							&& wsFolder != null && !wsFolder.getUri().isEmpty()) {


getUri() can return null, so we need another condition: !wsFolder.getUri() != null

Added as part of tidy up (see below).

rubenporras · 2022-05-09T10:43:24Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

+							|| (delta.getKind() == IResourceDelta.CHANGED
+									&& (delta.getFlags() & IResourceDelta.OPEN) == IResourceDelta.OPEN))
+									&& !project.isAccessible()
+									&& wsFolder != null && !wsFolder.getUri().isEmpty()) {


getUri() can return null, so we need another condition: !wsFolder.getUri() != null

Not according to LSP specification. So a LS that does return null here is not compliant with the spec.

Trivial to do, so added the guard anyway as part of the tidy up.

rubenporras · 2022-05-09T10:46:06Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

+							|| (delta.getKind() == IResourceDelta.CHANGED
+									&& (delta.getFlags() & IResourceDelta.OPEN) == IResourceDelta.OPEN))
+									&& !project.isAccessible()
+									&& wsFolder != null && !wsFolder.getUri().isEmpty()) {


this condition and the previous one are almost identical, could we refactor to a private method with a boolean flag isAccessbile?

It would be nice to break up the code a bit, but this class is already rather large, and so is e.g. LSPEclipseUtils. What would you say to replacing the lambda workspaceFolderUpdater at line 172 with a proper class implementing IResourceChangeListener, then all of this logic could be broken up and put there?

I guess it could be a nested class in here so that it could still have direct access to the LanguageServerWrapper.languageServer field, but it would maybe be nicer to break it out completely?

There was a race condition on shutdown where languageServer could become null on a worker thread just before a resource event is fired on the event thread. I moved the de-registration above line 456 in stop() but maybe a null guard in the workspaceFolderUpdater body wouldn't hurt?

I have no strong opinion about refactoring to a nested class. Regarding the null check, I think you need it in any case because when a project is deleted the URI will be null. I think this can happen regardless of the de-registration in stop().

mickaelistria · 2022-05-09T10:49:33Z

Sorry for my previous message, build failure was not related.

ahmedneilhussain · 2022-05-09T14:54:59Z

Sorry for my previous message, build failure was not related.

No problem - it was in a suspiciously related area of functionality!

mickaelistria · 2022-05-09T18:13:03Z

@ahmedneilhussain all those are good ideas. However, please create separate pull request for each of them as much as possible. Smaller PRs allow faster progress.

mickaelistria · 2022-05-10T09:56:41Z

@rubenporras @ahmedneilhussain Do you think this pull request is fine to merge in this current state, or are there some necessary changes left to implement ? (and refactoring and other stuff can be considered in further PRs)

…licated boolean logic to be broken up into nearby helper methods.

ahmedneilhussain · 2022-05-10T11:48:49Z

@ahmedneilhussain all those are good ideas. However, please create separate pull request for each of them as much as possible. Smaller PRs allow faster progress.

Sorry for slow response guys, I had today off officially but decided it was only a little bit of work just to tidy the code into a separate class.
What do you guys think @mickaelistria @rubenporras ? I think this is much cleaner and will be much easier to read and follow in future, particularly if we need to add support for move/rename (which I hope to be able to offer at some point). The logic for decoding and translating the eclipse resource change flags is particularly hairy and is a lot easier to read with meaningfully named helper methods.
Ideally I'd have the whole thing as a freestanding class but I wasn't in a rush to expose the LanguageServerWrapper.languageServer field just for this.

mickaelistria · 2022-05-10T13:59:46Z

Thanks!

rubenporras · 2022-05-10T14:40:29Z

@ahmedneilhussain , since Mickael already reviewed and merged I am going to assume it is all fine :)

mickaelistria · 2022-05-10T14:45:43Z

I think it was already fine for a few interations, so I merged it so it can be easily tested and further improvements are still welcome in further PRs.

rubenporras · 2022-05-11T08:13:04Z

Yes, the change looked good also to me, I just wanted to say that since you did the review already I would not look at it in much detail, since I did not think that would be needed.
Thanks

ahmedneilhussain added 2 commits April 28, 2022 16:01

Fix workspace folder events to use eclipse resource change flags corr…

a28fb70

…ectly. Avoid sending events with null content.

Add tests for WorkspaceFolderEvents

8e161a6

Tweak mock server initialisation so that the capabilities can be customised Move the logic for unsubscribing from Eclipse resource events a bit higher up LanguageServerWrapper.stop() to avoid potential race condition and NPE

mickaelistria reviewed May 6, 2022

View reviewed changes

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/WorkspaceFoldersTest.java Show resolved Hide resolved

Add standard EPL-compliant header

7400bfb

rubenporras reviewed May 9, 2022

View reviewed changes

Workspace Folders: extract resource listener into class to allow comp…

8bd9ee0

…licated boolean logic to be broken up into nearby helper methods.

mickaelistria merged commit 9ef09a2 into eclipse-lsp4e:master May 10, 2022

ahmedneilhussain mentioned this pull request May 23, 2022

Concurrency fixes #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WorkspaceFolders: Fix interpretation of event flags #116

WorkspaceFolders: Fix interpretation of event flags #116

ahmedneilhussain commented May 6, 2022

mickaelistria left a comment

mickaelistria commented May 9, 2022 •

edited

Loading

rubenporras May 9, 2022

ahmedneilhussain May 9, 2022

rubenporras May 10, 2022

mickaelistria May 10, 2022

ahmedneilhussain May 10, 2022

rubenporras May 9, 2022

ahmedneilhussain May 10, 2022

rubenporras May 9, 2022

mickaelistria May 10, 2022

ahmedneilhussain May 10, 2022

rubenporras May 9, 2022

ahmedneilhussain May 9, 2022

ahmedneilhussain May 9, 2022

ahmedneilhussain May 9, 2022

rubenporras May 10, 2022

mickaelistria commented May 9, 2022

ahmedneilhussain commented May 9, 2022

mickaelistria commented May 9, 2022

mickaelistria commented May 10, 2022

ahmedneilhussain commented May 10, 2022

mickaelistria commented May 10, 2022

rubenporras commented May 10, 2022

mickaelistria commented May 10, 2022

rubenporras commented May 11, 2022

WorkspaceFolders: Fix interpretation of event flags #116

WorkspaceFolders: Fix interpretation of event flags #116

Conversation

ahmedneilhussain commented May 6, 2022

mickaelistria left a comment

Choose a reason for hiding this comment

mickaelistria commented May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria commented May 9, 2022

ahmedneilhussain commented May 9, 2022

mickaelistria commented May 9, 2022

mickaelistria commented May 10, 2022

ahmedneilhussain commented May 10, 2022

mickaelistria commented May 10, 2022

rubenporras commented May 10, 2022

mickaelistria commented May 10, 2022

rubenporras commented May 11, 2022

mickaelistria commented May 9, 2022 •

edited

Loading