Repository navigation
Cancel pending change delayer on cleanup and handle re-entrant delayer tasks - #1920
Open
Abhinav Maurya (Fire162) wants to merge 1 commit into
Open
Abhinav Maurya (Fire162) wants to merge 1 commit into
Abhinav Maurya (Fire162) wants to merge 1 commit into
Conversation
…r tasks cleanUp() cancelled _fileEventDelayer but left _pendingChangeDelayer running. When a client stopped during the 250 ms debounce of a full text document sync, the armed timer fired after shutdown, attempted to re-trigger delivery without an active connection, and threw a TypeError because the running continuation cleared this.task after the re-entrant trigger. Cancel _pendingChangeDelayer in cleanUp() and detach this.task before running it in Delayer.trigger() and Delayer.forceDelivery().
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes address both reported failure causes, with no identified blocking issues in the supplied review evidence.
No open issues 🎉
What changed in this PR
This PR fixes the pending full-document-sync shutdown error reported in #1916 and makes the client’s Delayer safe for tasks that schedule another task.
Changes:
- Cancel pending change delivery during client cleanup.
- Clear the current delayer state before invoking tasks in
trigger()andforceDelivery(). - Add a regression test for a task that triggers the delayer again.
| File | Description |
|---|---|
client/src/common/utils/async.ts |
Preserve newly scheduled tasks during re-entrant delivery. |
client/src/common/client.ts |
Cancel the pending-change delayer during cleanup. |
client-node-tests/src/converter.test.ts |
Test re-entrant task scheduling. |
Review effort: Balanced
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When a client stops while an edit is debounced during full text document sync,
cleanUp()cancels_fileEventDelayerbut leaves_pendingChangeDelayerscheduled. When the timer fires without an active connection, it re-triggers delivery and hits aTypeError: this.task is not a functionbecause the running task continuation clearedthis.taskafter the re-entrant trigger.Cancel
_pendingChangeDelayerincleanUp()and detachthis.taskbefore running it inDelayer.trigger()andDelayer.forceDelivery().Fixes #1916