You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is a long running issue with Brackets where the async stat implementation for fetching the timestamp (stat._mtime) can cause rare 🐎 race conditions 🐎. See this issue: #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.
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:
// 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@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.
@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.
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.
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.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the same patch as #12175, but targeted against release branch.
🔥 Potentially fixes #11826 🔥
There is a long running issue with Brackets where the async
stat
implementation for fetching the timestamp (stat._mtime
) can cause rare 🐎race conditions
🐎. See this issue: #295This is very rare and usually doesn't matter much. But it does matter when
FileSystem.js
does this:Due to the race condition
stat.mtime
is sometimes older thanoldStat.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:
Editor#resetText
which calls: