Skip to content
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

Add a flow control mechanism to the attach addon #1918

Closed
jerch opened this issue Jan 31, 2019 · 4 comments
Closed

Add a flow control mechanism to the attach addon #1918

jerch opened this issue Jan 31, 2019 · 4 comments
Labels
area/addon/attach type/enhancement Features or improvements to existing features

Comments

@jerch
Copy link
Member

jerch commented Jan 31, 2019

The old flowcontrol approach with XOFF/XON does not work in envs that disable flow control on the pty. Since many shell startup scripts disable it, we can remove it from Terminal.write.

For local apps like electron based flow control can be achieved by avoiding async buffers in between, for remote apps we might want to add some flowcontrol mechanism to the attach addon.

@Tyriar Tyriar added type/enhancement Features or improvements to existing features addon help wanted labels Jan 31, 2019
@PerBothner
Copy link
Contributor

DomTerm keeps a running count of the number of bytes received and the number of bytes confirmed, which are tracked both in the browser and the (WebSockets) server. (Both of these counts are modulo 0x10000000 to deal with wrap-around.) When the difference between the number of received and confirmed bytes is larger than some threshhold (currently 500), then the browser sends a RECEIVED message with the current received count, and updates the confirmed count to match. This is done after processing the received bytes.

Similarly, the server keeps track of the number of sent and confirmed bytes. (The confirmed count is the most recent value of a RECEIVED message.) When the difference between these values gets above a certain threshold (currently 8000 bytes) then the server will disable polling of the file descriptor for the output from the pty. The user process will pause when the kernel tty driver buffers are full. When the browser has caught up, it will send a new RECEIVED message, and polling and reading of the pty will resume.

DomTerm uses a "report-event" escape sequence for messages sent from the browser to the server. The byte-stream from the browser to the server contains primarily user key-strokes (encoded as in xterm), as well as these "event" messages. A domterm server needs to scan the stream for event messages and handle them appropriately.

The xterm.js project concentrates on the browser part of the terminal, leaving the server part to embedders. I suggest adding an escape sequence that the server can send to notify xterm.js that the server understands these event messages, including the RECEIVED message.

@jerch
Copy link
Member Author

jerch commented Feb 3, 2019

Yeah we dont want to focus more on the server side as this is left to integrators (could be build in any language/framework/whatsoever). Thus we might need some general way to indicate the server part "hey - slow down abit".
An easy approach would be a high/low watermark buffer observer, that would just signal a "stop" at the high mark and a "go" at the low mark. Since we do not control the server part it would be free to ignore these signals and just keep on sending data. Not sure if we need to go that far and have to implement lossy data handling to deal with "malicious" servers (this certainly would save the xterm.js process from crashing).

We further might need to document some instructions for integrators how to achieve lossless handling of those "stop" and "go" signals in their server parts. In envs that cannot handle blocking things well ppl might struggle to set up things, esp. with nodejs and its non blocking control flow you can easily end up with exactly the same problem on server level. Maybe this is worth a wiki entry later on.

@Tyriar Tyriar changed the title revisit flowcontrol Add a flow control mechanism to the attach addon Feb 5, 2019
@Tyriar
Copy link
Member

Tyriar commented Oct 7, 2019

@jerch correct me if I'm wrong but I think we landed on having some general guidance for flow control instead of implementing it within attach? It's relatively simple to do now that we have the write callback.

@Tyriar Tyriar removed the help wanted label Oct 7, 2019
@jerch
Copy link
Member Author

jerch commented Jan 5, 2020

We should not provide an out-of-the-box flow control mechanism in the addon, it depends way to much on the actual backend used and needs additional server code to work properly. Thus closing the issue.

@jerch jerch closed this as completed Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon/attach type/enhancement Features or improvements to existing features
Projects
None yet
Development

No branches or pull requests

3 participants