Always close editor when file is deleted

[tobous]

[INTERNAL][E] Add logic to close editor without saving

Preparation for fixing #758.

[FIX][E] #758 Always close editor when file is deleted

Adjusts the logic handling file deletion actions on the sending and
receiving side. Ensures that the editor for the deleted file is always
closed properly without saving.

Skipping the save is important as the editor will only still be open for
the deleted file if it was in an unsaved state at the time of deletion.

Closing such unsaved editors for deleted files is important as they
otherwise create issues with the consistency logic. Since the watchdog
logic is based on the open editors, the remaining editor is interpreted
as the resource still being present, leading to an inconsistent state.

Fixes #758.

[srossbach]

As I side note although it is not obvious. The code executed here should (regarding the Eclipse documentation) not do heavy work, we already do heavy work here and even sync on the SWT thread which is deadlock prone (it currently does not happen). I do not know what happens if you invoke Eclipse logic here.

And the other question ? When does this even happen. If I delete a file that is dirty (the editor) Eclipse closes the editor. So this can just happen when this is automated, e.g it could caused by another plugin (maybe by accident).

[tobous]

As I side note although it is not obvious. The code executed here should (regarding the Eclipse documentation) not do heavy work, we already do heavy work here and even sync on the SWT thread which is deadlock prone (it currently does not happen). I do not know what happens if you invoke Eclipse logic here.

I added the logic here as it was the obvious spot to put it. I am not familiar with the Eclipse event listener logic. Do you know of a better way of reacting to a local file deletion which allows me to do "heavy work".

And the other question ? When does this even happen. If I delete a file that is dirty (the editor) Eclipse closes the editor.

My assumption was that it happens when eclipse encounters an editor on deletion that is still dirty after the warning dialog for a dirty editor (i.e. the dialog saves the editor to allow for a clean deletion but our activities make it dirty again before the actual deletion takes place). But I am not completely sure. An "easy" test setup that let me reproduce the issue consistently is:

• Start a session between Alice and Bob, both running on different machines
• Both open an editor for the same file
• Alice creates continuous uninterrupted inputs in the editor (i.e. holds down a keyboard key)
• Bob deletes the shared file for the editor
  (Weirdly enough, Eclipse does not always let you deleted files with a dirty editor. This only seems to be possible after the file was saved at least once locally (remote saves do not count) after it was created.)

The result of this is that Alice or Bob (somewhat inconsistent) still has a dirty open editor for the deleted file, even though it no longer exists on disk.

This behavior no longer appears with this workaround.

So this can just happen when this is automated, e.g it could caused by another plugin (maybe by accident).

My local Eclipse installation is just the default "Eclipse IDE for Eclipse Contributes" (release 2019-09). I don't have anything else installed besides "GEF Legacy". So even if this issue is caused by another plugin, it is most likely a pre-installed one (at least for the contributes edition), meaning we should handle the issue either way in my opinion.

[srossbach]

The scenario that you described should be fixed with the modification you made to FileActivityConsumer. I do not recommend to close editors in the ProjectDeltaVisitor as you do not know what caused the deletion of the file.

[tobous]

The scenario that you described should be fixed with the modification you made to FileActivityConsumer.

But isn't the FileActivityConsumer only responsible for incoming deletion activities? I distinctly remember also having the same issue on the side of the participant deleting the resource. (On the other hand, I could not recreate this during my recent tests. Only the receiver got the issue, which would match your suggestion.)

I will do some more testing with and without this part of the fix and report back.

[tobous]

I can confirm that the issue still occurs on host side after removing the added line from ProjectDeltaVisitor. With the line, I was not able to reproduce the issue in my second test session.

Sadly, I haven't found a way of consistently triggering the issue. With the described setup, it only works about 30-40% of the time.

I had a look at the logs but couldn't find anything out of the ordinary besides some jupiter vector time errors (some of which were caused by the faulty heartbeat logic; also fixed by #714). But I only looked at loglevel warning and above (as Saros/E logs way to much as trace is enabled by default for the devel build).

I tried it a bunch of time (as it is inconsistent), meaning the log files contain over 10000 lines. But if you want, I can still upload them so you can have a look at them.

[srossbach]

Looking at the log:

delete file
-> send delete file
<- receive editor closed
execute documentprovider disconnect which results in releasing the handle to the buffer

Just wondering what will happen if we release the handle(s) before the file is deleted. I guess this would also fix some stuff regarding the watchdog logic, e.g imagine you send a file delete but never receive and editor closed for this activity, the buffer is still read.

[tobous]

Ok, I am not really sure what to do with that information. Could you provide a patch/PR that should fix the underlying issue? If you prefer, I can then still test it for you. Or we could develop/test/debug it together.

[srossbach]

After thinking about this issue again it seems this is currently the best option to solve this problem. It also ensures that an EditorClosedActivity is send before the FileDeletionActivity is executed. Lets hope it do not cause any deadlocks.

[tobous]

Would spawning another thread and executing the editor close on it help, just to not run it in the thread the ProjectDeltaVisitor is run on?

This would remove the "synchronous" part of the execution, meaning the editor closed activity would not necessarily be send before the deletion activity. But I don't know how much the exact timing/order of the two activities matters.

[tobous]

Would spawning another thread and executing the editor close on it help, just to not run it in the thread the ProjectDeltaVisitor is run on?

I will merge this for now. If you do prefer a solution utilizing another thread, I can still add it in a separate PR.

[tobous]

I created the issue #758 for this and mentioned it in the commit messages to make it easier to understand issue/reasoning behind the commit when looking at the commit later on.