-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[filesystem] Fix missing "out of sync error" #7139
Conversation
Just had to check now why we broke it, probably we tried to fix something else :) |
ok, it seems i overlooked it by while introducing incremental updates: bace685#diff-bc50ceab9ceca36de64ebebbe8a2987bL64 How did we live with that for such long time 😬 |
It does not work very well :( It does ask, but if i say don't save then it asks again and then it does not allow me to save anymore. |
It would be good to create integration tests for it as well. |
@AlexTugarev Can i push to this PR? |
Please, yes! |
f076519
to
574bf69
Compare
@AlexTugarev i'm going to close it and open a new PR, fine? that we can revert author and reviewer roles? |
574bf69
to
5a80e67
Compare
@AlexTugarev It's turned out to be really complicated. The main issue is that monaco model does not know about resource stat while dirty but it still can fetch a resource and reset it to latest stat because of it. In such case, monaco model will overwrite fs. I wonder should we expose something like stat on resource. btw I've added integration tests for saving to make it reproducible: https://github.com/eclipse-theia/theia/pull/7139/files#diff-74f4047f6db63505c37031b5e5228464R66-R124 |
Discussed with @svenefftinge that we will add optional version attribute to Resource which clients can pass to save operation. |
Also noticed that on reload deleted files are not marked as deleted and overwrite content on the disk if saved after file is added again to the disk in background. Also add tests for:
|
e3a44d1
to
8505ac1
Compare
bc383db
to
f3dd0a9
Compare
@AlexTugarev @marcdumais-work @vince-fugnitto @lmcbout Could you test please? I've added integration tests for all kind of cases of save cycle, but it would be good to test manually as well that saving works well generally and against changed state on the disk. |
@marcdumais-work could you try again please? |
There is still something fishy with move of dirty editors when I test with Java, now editor is properly moved, but there is an error in the editor. It looks like Java LS is missing some updates for the target editor. |
Works better now - the problem I had earlier is fixed. Will test some more. |
Seems to work well (same as master):
Seems to work well (same as master):
With this PR the editor for the deleted file is not restored after reload 👍
On its own, autosave works fine as far as I can tell. However if we add concurrent modifications done from outside Theia, it does not work great compared to VS Code. (but no worse than theia master as far as I can tell). So maybe this could be tackled separately. The result seems to depend on the timing of auto-save vs timing of outside modifications. It's very easy for the changes from outside to be silently overwritten without the user noticing it's the case if the changes are concurrent.
At times, depending on timing, I can see :
|
Do you test with auto on save in both in Theia and VS Code? |
yes, both. |
@marcdumais-work I cannot reproduce, i get consistently a dialog asking me to overwrite in Theia.
|
that sounds bogus, on move, old editor should be closed regardless whether dirty or not, target editor should be opened instead with the same state. It is bogus as it behaves on master, e.g. see #7054 @marcdumais-work Could you check that the build was successful for this PR? Maybe you were testing without changes applied? |
a838e0c
to
adf9023
Compare
@AlexTugarev Could you have a look whether you can reproduce @marcdumais-work issues? See #7139 (comment) |
@marcdumais-work and @AlexTugarev, in the test from above where you're trying to catch periodic changes from outside, that's actually what I would even assume what should happen, given:
editthat's actually not what the ☝️ comment was about. Anton just explained, that it's about the dirty state, etc. 🤦♂ nvm! |
@marcdumais-work I re-tested the "outside-changes" scenario with auto-save enabled from above, and it looks good. are you 100% sure, that it's not the master state you're looking at? I tried several times to make sure not posting bs again ;-) |
@akosyakov that "deleted from disk" still exists when using |
@AlexTugarev I know, we don't participate in move from outside, only when they triggered by Theia, there is no reliable way to detect it. It is the same on master. I'm more concerned with cases when we programmatically rename or move something, like in #7054 Could you file a new issue? I'm not sure that it is solvable since we have to take snapshot of the state before move and events come only after. |
@akosyakov thanks for clarification! |
@marcdumais-work, it would be great if you can retest 🙏 This PR solves a lot of issues already. |
which should be shown if one try to save pending editor changes to a file which was modified in background. Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
adf9023
to
e4d8b54
Compare
Hi @AlexTugarev , @akosyakov , Back to work this morning. I pulled the latest version of this PR and it's building. Let me retest a bit and comment.
👍 |
Update: I can't reproduce the "concurrent file update" issue. It's possible that there is a race-condition in the scenario where we may wrongly overwrite outside updates, but if so it's not so easy to trigger anymore.
I confirm that's what happens when renaming a dirty file from inside Theia 👍 As I think @AlexTugarev figured, I was originally renaming the file from the terminal using Conclusion: looks good to merge, behavior-wise. |
@marcdumais-work @AlexTugarev thank you for the review could someone approve please? I've also pushed one more commit to insulate tests against nsfw failures: bd6f947 It should make sure that file resource operates even if nsfw failed given that file operations are triggered via Theia UI (FileSystem API). If not then we are out of luck if nsfw failed. It seems to work. There are nsfw failures on travis (https://travis-ci.com/eclipse-theia/theia/jobs/288440321#L6249), but tests don't fail this time, i.e. file resources keep operating. |
@AlexTugarev said (offline) that he will review the code once |
Integration tests revealed that nsfw fails from time to time as well as introduce delays. It affects the file resource especially for the delete operation. Instead of relying only on nsfw, file resource now listens to file delete triggered by a user from Theia. It allows to react faster as well as tolerate failures of file watching. Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
bd6f947
to
3474d18
Compare
✔️ I went through the changes and tested once again. It looks very good! Thanks for investing so much time into this issue, Anton! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved on behalf of @AlexTugarev, since we changed roles: #7139 (comment)
@marcdumais-work is it good to merge with you?
which should be shown if one try to save pending editor changes to a file which was modified in background.
Fixes #7102
Thanks @akosyakov to provide this fix!
How to test
Review checklist
Reminder for reviewers