[CLOSED] [HOTFIX] Only update stat and clear contents when old stat is newer than current stat.

core-ai-bot commented 3 years ago

Issue by petetnt Friday Feb 05, 2016 at 07:03 GMT Originally opened as https://github.com/adobe/brackets/pull/12195

This is the same patch as #12175, but targeted against release branch.

:fire: Potentially fixes #11826 :fire:

There is a long running issue with Brackets where the async stat implementation for fetching the timestamp (stat._mtime) can cause rare :racehorse: race conditions :racehorse:. See this issue: https://github.com/adobe/brackets/issues/295

This is very rare and usually doesn't matter much. But it does matter when FileSystem.js does this:

      // Update stat and clear contents, but only if out of date
      if (!(stat && oldStat && stat.mtime.getTime() === oldStat.mtime.getTime())) {
         entry._clearCachedData();
         entry._stat = stat;
         this._fireChangeEvent(entry);
      }

Due to the race condition stat.mtime is sometimes older than oldStat.mtime, causing a change event to happen and cache get cleared, nullifying the history -> causing #11826.

Q & A

Q: Brackets has been like that for a long time. Why did this start occurring in `1.5.`?

A: I only got speculation, but as it is a race condition, I guess increased code base plays an unfortunate part here: some additional file system operations or anything else made the race condition much more reoccurring.

Q: Does this really fix #11826

A: I... don't know for sure. There's no steps to reproduce #11826 with even 1% success rate

Q: Why do you think it fixes #11826

A: It fixes the issue with my corrupt-brackets-history extension: I tried to save 10000 times in a row and history was kept intact. Without the fix the issue usually occurs before the 50th save.

Q: Does this have other side-effects

A: Not sure. Most likely not, but I am a bit worried that it might have unwanted effects on some external changes. Obviously it should be better to fix the race condition itself, but this is just a hotfix.

Q: Why is the history getting reseted

A: The change event getting erroneously called eventually leads to this chain:

FileSyncManager#reloadChangedDocs 
-> FileSyncManager#reloadDoc
-> Document#refreshText 
-> Editor#resetText

Editor#resetText which calls:

 // Make sure we can't undo back to the empty state before setValue(), and mark
// the document clean.
this._codeMirror.clearHistory();
this._codeMirror.markClean();

petetnt included the following code: https://github.com/adobe/brackets/pull/12195/commits

core-ai-bot commented 3 years ago

Comment by lkcampbell Tuesday Feb 23, 2016 at 16:39 GMT

@petetnt@abose What's the update on this fix? I know we don't have a lot of feedback on benefits and problems of merging it, but all the feedback we do have is positive. Is this going in for 1.7? If so, we should at least assign it a milestone.

core-ai-bot commented 3 years ago

Comment by petetnt Tuesday Feb 23, 2016 at 21:39 GMT

@lkcampbell FWIW I have been using it since I figured it out and haven't ran into the undo-issue myself nor into any bugs caused by it. Like you said, the other feedback we have gotten is rather minimal but at least 100% positive.

core-ai-bot commented 3 years ago

Comment by redmunds Tuesday Feb 23, 2016 at 22:58 GMT

This looks good to me.@peterflynn Can you think of any problem that this very simple change could cause?

I am curious as to why this PR is targetting the release branch because I don't think it's active. This change should be made in master branch, right?

core-ai-bot commented 3 years ago

Comment by abose Wednesday Feb 24, 2016 at 05:05 GMT

We were thinking of triggering a minor release by updating the 1.6 release builds with the patch as this is a critical issue. This PR was initially targetted to master. But as it is already time for the next release, maybe this will go to 1.7. Discussions still going on as triggering a release process is a bit expensive even for a minor release.

core-ai-bot commented 3 years ago

Comment by petetnt Wednesday Feb 24, 2016 at 07:12 GMT

Closing this as it landed to master on #12175

core-ai-bot commented 3 years ago

Comment by abose Wednesday Feb 24, 2016 at 09:58 GMT

We will be doing a minor build refresh of 1.6 with this fix soon. Merging.

core-ai-bot commented 3 years ago

Comment by petetnt Wednesday Feb 24, 2016 at 10:07 GMT

@abose Would it be possible to include #12145 in the refreshed release? It has prompted 10+ new issues after the launch of 1.6.

core-ai-bot commented 3 years ago

Comment by abose Wednesday Feb 24, 2016 at 10:49 GMT

We had a discussion about this; But putting in too many bug fixes in a build refresh will increase the testing overhead as it is a core change. I am up for taking #12145 in the refresh. Tagging@swmitra for opinion.

core-ai-bot commented 3 years ago

Comment by swmitra Wednesday Feb 24, 2016 at 16:28 GMT

@abose@petetnt We should include #12145 for the refresh.

brackets-archive / bracketsIssues