-
Notifications
You must be signed in to change notification settings - Fork 246
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
flow control handling #304
Conversation
src/interfaces.ts
Outdated
@@ -115,6 +115,9 @@ export interface IPtyForkOptions { | |||
uid?: number; | |||
gid?: number; | |||
encoding?: string; | |||
handleFlowControl?: boolean; | |||
flowPause?: string; |
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.
This probably doesn't need to be configurable? If people want it we could always add it at a later point if there's the need.
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.
Well if we want this to work in xterm.js with zsh where ppl tend to rebind XON/XOFF (those are the defaults) we already have that use case?
src/terminal.ts
Outdated
return; | ||
} | ||
this._realWrite = this.write; | ||
this.write = (data: string) => { |
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.
Maybe write should be a (data: string) => void
property on terminal.ts and it can be assigned by unix/windowsTerminal? Feels so dodgy monkey patching class methods.
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.
Sounds good.
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.
Changed it this way: write
is now provided as default impl flowcontrol aware, the derived classes instead set _writeMethod
to their impl. _writeMethod
gets called by Terminal.write
later on.
Tests are still passing 😄
src/terminal.ts
Outdated
/** | ||
* Disable automatic flow control handling. | ||
*/ | ||
public disableFlowHandling(): void { |
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.
Toggling this after creation seems like something no one will need?
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.
Ah this will be needed for binary transport (for things like zmodem protocol). Binary data tend to contain any sequence by accident, thus ppl need a possibility to disable (and later reenable) it.
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.
How would it work at all with zmodem then?
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.
zmodem would search for XON/XOFF and escape it with ZDLE char (xor'ed?). Not sure about the implications but I think the control codes are used for its message framing, too. Cannot make much sense from the lines here: https://stackoverflow.com/a/9710116
- default write is provided by Terminal with flowcontrol - derived classes attach their write implementation to _writeMethod
This PR introduces automatic flow control handling for node-pty.
Instead of relying on XON/OFF flow control is done by
pause
andresume
of the underlying socket. This way we can ensure it to work even if XON/XOFF is not available.The PAUSE/RESUME messages excepted default to XON/OFF control codes. Since some ppl customize these control codes the messages can be changed via ctor options or arguments for
enableFlowControlHandling
.@Tyriar: Also wrote test cases, but those still need to be adopted for windows.