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

flow control and discard watermark #2295

Closed
wants to merge 23 commits into from
Closed

Conversation

jerch
Copy link
Member

@jerch jerch commented Jul 9, 2019

Second attempt to solve the flow control issues. Other than the first attempt this relies on ENQ --> ACK cycles:

  • server sends request (ENQ) every n-th bytes
  • xterm.js responds with answerbackString

xterm.js handles the ENQ request in-band with the escape sequence parser, thus a response guarantees that previous data already got processed. With this the server side can implement strategies to measure data throughput and can pause the pty if needed.

The demo server implements a simple pending ACK strategy: Every n-th data bytes it sends an ENQ and peeks for ACK responses. If the pending requests hit a limit the pty gets paused.

Note: The request message is hardcoded to ENQ in xterm.js, the response message can be cumstomized by the option answerbackString (as suggested by xterm docs).

Again this PR comes with a discard parachute to avoid crashing the browser. It is hardcoded to ~50MB.

src/Terminal.ts Show resolved Hide resolved
demo/client.ts Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
demo/server.js Outdated Show resolved Hide resolved
demo/client.ts Show resolved Hide resolved
demo/client.ts Outdated Show resolved Hide resolved
src/common/services/OptionsService.ts Outdated Show resolved Hide resolved
@Tyriar Tyriar added this to the 4.0.0 milestone Jul 10, 2019
demo/client.ts Outdated Show resolved Hide resolved
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I ctrl+c yes it stop doing anything:

image

EDIT: Oh got that working.

demo/server.js Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Jul 11, 2019

Did the state tests for ENQ and well, we cannot rely on a C0 byte for this as they are typically ignored / valid payload for the subparser states OSC, DCS, APC, PM and SOS.

To keep going with a simple one byte in-band variant we would have to go with an unused C1 byte. Hottest candidates for this are PU1 or PU2 (meant for private usage).

Alternatively we could go with a totally custom sequence (more bytes allowed), but would have to patch the parser to carry forward the previous state to be re-applied afterwards.

Already time to dump the idea to handle it on VT parser level and go with transport level instead? Well it kinda belongs there and not in the VT parser, but would need us to introduce a tiny "protocol" to wrap the payload and to implement a backpropagation from the internal write buffer states. By doing it right at the parser level we can skip this complexity and perf penalty. Hmm.

To circumvent the perf penalty of wrapping every payload chunk into some custom protocol, we could go back to our original idea of a one-sided report: Whenever xterm.js finished processing of xy bytes it could just announce that to the backend. The backend then can approximate whether xterm.js is to far behind and pause/resume the pty.
The ideal solution would apply this to both directions, but I think we dont need that for frontend to backend communication.

Last but not least we also should take into account that other things might want to "abuse" the backend-frontend connection for their stuff (like zmodem, or embedding ideas like #2300) - this has to work for those too without complicating things on xterm.js and backend side to much.

@jerch
Copy link
Member Author

jerch commented Jul 13, 2019

@Tyriar Done with the basic rewrite. What we have now:

  • optional callback on write, writeUtf8 and writeln
  • callback get triggered right after the chunk was parsed, implications: chunk is guaranteed to be applied to offscreen parts of the terminal (parsing is sync code down to the buffer), still this does not apply to screen state, which gets not updated before the next event loop invocation (does not matter for the flow control issue)

With these changes the inner write states can be tracked. Now its easy to decide whether the terminal is under high data pressure and falling behind, thus flow control can be implemented.

The demo implements flow control as following:

  • attach addon:
    • tracks incoming data size
    • places every n-th bytes a callback into the write chain (customizable, 0 for disabled)
    • callback sends ACK reply back to the server
  • server,js:
    • tracks outgoing data size
    • expects an ACK reply for every n-th byte (must be in sync with attach addon setting!)
    • does high/low watermark on pending ACKs (pause pty at high, resume pty at low watermark)

To avoid problems on implementors' side the flow control setting in the attach addon is disabled by default.

New Protocol:
ACKs are sent in-band over the websocket to avoid latency problems with a second (TCP) connection. To achieve this a tiny protocol was built on top to allow different message types (this is needed since the data stream can contain arbitrary bytes). I think this protocol will also come handy for other stuff later on, like in-band resize updates and raw mouse reports (we have problems with the mousey for non utf8 reports, see #1962). Thus its active by default.

About the ENQ:
Not sure what to do with this, imho its ancient feature we shortly revived for the wrong purpose. Still we get one more point in vttest rating with it - maybe move to the yet to come compat addon?

Well slowly getting there, up for a review.

Edit: Btw, foreign things like zmodem transport over the same websocket should be easy now. The data sink (zmodem) would just have to announce data handling back like xterm.js does, thus trigger an ACK reply every n-th bytes, so the server part can do the ACK accounting as with a normal terminal stream.

@jerch
Copy link
Member Author

jerch commented Jul 21, 2019

@Tyriar Looked further into the UTF8 problem with arbitrary data #2326 and come to the conclusion that we should give up on string based transport for data from browser to server.js to avoid the (faulty) utf8 conversions. Thus I created a binary version of the protocol.
I think we need to further discuss how to layout this, removing string transport from the attach addon might break with many installations. Still I see more reasons to drop it.

@jerch jerch mentioned this pull request Aug 7, 2019
@jerch
Copy link
Member Author

jerch commented Aug 7, 2019

@Tyriar:
Should we close this PR in favor of #2362 and a separate attach addon PR later on?

@jerch jerch removed this from the 4.0.0 milestone Aug 24, 2019
@Tyriar
Copy link
Member

Tyriar commented Sep 6, 2019

Yes let's close this, we also need to remove some stuff in node-pty since our thinking changed right?

@Tyriar Tyriar closed this Sep 6, 2019
@Tyriar Tyriar added the reference A closed issue/pr that is expected to be useful later as a reference label Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reference A closed issue/pr that is expected to be useful later as a reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants