-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
hooks for custom control sequences (updated) #1853
Merged
Merged
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
4d660de
hooks for custom control sequences
PerBothner b689745
Cleanups required by tslink.
PerBothner 0311563
Optimize parsing of OSC_STRING to minimize string concatenation.
PerBothner 30a667c
Revert "Optimize parsing of OSC_STRING to minimize string concatenati…
PerBothner 8ad2d1b
Merge remote-tracking branch 'upstream/master'
PerBothner ffb2708
Revert "Cleanups required by tslink."
PerBothner 53fd04a
Revert "hooks for custom control sequences"
PerBothner 8ceea11
hooks for custom control sequences
PerBothner 6351a5b
Merge branch 'master' into master
Tyriar 5af4626
Change addCsiHandler/addOscHandler to not use Object.assign.
PerBothner 8a5a032
New method _linkHandler used by both addCsiHandler and addOscHandler.
PerBothner 6b65ebd
Various typing and API fixes, doc comments, typing test etc.
PerBothner 29cc0bf
Merge remote-tracking branch 'upstream/master' into control-seq-handler
PerBothner 48ff841
Be more paranoid about cleaning up escape sequence handlers.
PerBothner a80baa7
Merge branch 'master' into control-seq-handler
Tyriar d01efdd
Use array instead of linkedlist, add typings
Tyriar 38796a0
Add tests, fix NPE
Tyriar c045c80
Add tests for dispose
Tyriar adbb929
Wrap .d.ts comments to 80 chars
Tyriar 51e1f49
Make dispose more resilient
Tyriar bbfe149
Merge pull request #1 from Tyriar/hooks_changes
PerBothner 48c1d36
Merge branch 'master' into control-seq-handler
Tyriar 8fbeadd
Add missing deleteCount argument to Array.splice calls.
PerBothner f534758
Merge branch 'master' into control-seq-handler
jerch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
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.
nextHandler
gets not freed anywhere possibly holding that handler forever (if the disposed handler gets not removed on caller side). Same with thehandlers
object. Imho nulling both afterwards and conditionally exiting at the beginning when they not exist anymore (already disposed) solves this.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.
"nextHandler gets not freed anywhere possibly holding that handler forever (if the disposed handler gets not removed on caller side)."
Right - but wouldn't that be a leak on the caller side? You really shouldn't keep a reference to
foo
after callingfoo.dispose()
. We can null out nextHandler as a precaution, but it would only serve to paper over a leak in the caller, methinks:"Same with the handlers object."
I don't think that is an issue. The
handlers
object isthis._csiHandlers
orthis._oscHandlers
, which are both allocated in the constructor.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.
Yepp thats basically a leak on caller side, but in JS ppl tend not to think about lifetime of an object at all. And we cannot control caller side here as its exposed to the public. Therefore I think we should take care over what we can control as a precaution. Both refs are likely to contain handlers of
InputHandler
which itself contains aTerminal
ref.Terminal
is kinda the root of our "disposable object tree", thus forgetting to remove a disposed handler ref might prevent the whole object tree from GCing, which is far worse than just a leftover handler ref.Now talking about dispose and GC - I think it is also flawed regarding the invocation on a higher tree object that takes care of the subtree objects (the default
dispose
descent). I thinkEscapeSequenceParser.dispose
also would have to invokedispose
on the added handlers (with nulling in place), otherwiseInputHandler
will be kept bound in the added handlers. (InputHandler.dispose
itself nulls the terminal object, so at least the majority of objects will be released.)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.
Additional note on that: From API perspective its a surprising side effect, that an added handler pulls in refs to the terminal object (through
handlers
andnextHandler
). Its not obvious for ppl, they only see the interface that acts on parser states and input. Atmdispose
covers the "remove" thing, but not the ref releasing.Btw the arrow function as added handler binds the parser object as
this
, just to be able to call the fallback. Hmm. Thats not the case for handlers viaset...
. Imho this needs a slightly different approach.