No need to run in workspace for didSave

[mickaelistria]

The only workspace operation is a potential refresh; no need to wrap it in a job that may become a blocker.

[rgrunber]

We have a few places in the code where we do this (did{Open,Close,Change}) but I'm assuming we do other changes there so locking the resource is likely needed. Only thing that also stands out is that AVOID_UPDATE prevents resource change notifications from triggering during the operation. If it's just a refresh I guess that shouldn't be an issue.

@snjeza , what about https://github.com/eclipse/eclipse.jdt.ls/blob/40611dc3e419821aa76f58c54314a7d23e9a059a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java#L486-L487 ? Is this change safe with this operation ?

[snjeza]

? Is this change safe with this operation ?

The checkPackageDeclaration method can call the file.delete() method. See https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java#L531
I guess we should run it in the workspace.

[mickaelistria]

OK, I'll try to refine some preconditions to determine whether the workspace run is actually necessary. For example, in case the resource is already up-to-date, the workspace run becomes a bottleneck for other operations, with a risk of deadlock.

[rgrunber]

@snjeza , is there anywhere else we could move that checkPackageDeclaration operation ? Maybe to didChange.. or willSaveUntil ? As far as I understand, the purpose of that check is to re-adjust the source file in an invisible project based on the package declaration, since initially it's created against the default package.

If we could move that, that would leave didSave effectively just doing refreshes.

[snjeza]

is there anywhere else we could move that checkPackageDeclaration operation ? Maybe to didChange.. or willSaveUntil ? As far as I understand, the purpose of that check is to re-adjust the source file in an invisible project based on the package declaration, since initially it's created against the default package.

We could try to move it to handleChanged.

[rgrunber]

I think that's worth trying, if it improves the situation on other clients. I would verify how the behaviour changes when moving checkPackageDeclaration into didChange. I assume it would attempt to update the location the moment a valid package declaration is typed out. Hopefully this doesn't result in it performing multiple actions per keystroke. Also why I considered maybe adding it to willSaveUntil.

[rgrunber]

I don't mind having this live on as an issue that we can maybe look at. However, if you have a workaround, I think this should be lower priority now.

[mickaelistria]

I don't mind having this live on as an issue that we can maybe look at. However, if you have a workaround, I think this should be lower priority now.

Currently, I have more a fork than a workaround; but that's good enough to me at the moment.
However, I still think that if JDT-LS can work without this locked workspace, it's beneficial in general as some operations may then be faster.

[mickaelistria]

@rgrunber do you think this can be merged?

[mickaelistria]

I will try to create a test case that highlights the case when this workspace.run() is necessary. So it will verify this change doesn't cause a regression, and if it does, it will allow to make the proper fix.

[mickaelistria]

I've updated this PR to narrow the cases where the workspace.run can be avoided.

[rgrunber]

@snjeza , can you verify whether we can make this change ? To me it seems like checkPackageDeclaration only acts on files within the default project, so if the Uri being passed to didSave is not related to the default project, it should be safe to run it outside of the workspace runnable job.