-
Notifications
You must be signed in to change notification settings - Fork 6
Add reconnecting ptys #23
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
Merged
Merged
Conversation
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
code-asher
commented
Apr 27, 2022
smallnest/ringbuffer requires you to manually reset.
I got this up in my v1 deployment and it seems to be working! |
9743693
to
a862b70
Compare
kylecarbs
approved these changes
Apr 29, 2022
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.
Good tests, good code!
code-asher
added a commit
that referenced
this pull request
Sep 14, 2022
This partially reverts commit 9120171. The new method using screen will not share processes which is a fundamental shift so I think it will be easier to start from scratch. Even though we could keep the UUID check I removed it because it seems cool that you could create your own sessions in the terminal then connect to them in the browser (or vice-versa).
code-asher
added a commit
that referenced
this pull request
Nov 22, 2022
* Refactor reconnect test to support sub-tests Going to add an alternate screen test next. * Revert "Add reconnecting ptys (#23)" This partially reverts commit 9120171. The new method using screen will not share processes which is a fundamental shift so I think it will be easier to start from scratch. Even though we could keep the UUID check I removed it because it seems cool that you could create your own sessions in the terminal then connect to them in the browser (or vice-versa). * Add test for alternate screen The output test waits for EOF; modify that behavior so we can check that certain strings are not displayed *without* waiting for the timeout. This means to be accurate we should always check for output that should exist after the output that should not exist would have shown up. * Add timeout flag to dev client This makes it easier to test reconnects manually. * Add size to initial connection This way you do not need a subsequent resize and we can have the right size from the get-go. * Prevent prompt from rendering twice in tests * Add Nix flake * Propagate process close error * Implement reconnecting TTY with screen * Encapsulate session logic * Localize session map * Consolidate test scaffolding into helpers I think this helps make the tests a bit more concise. * Test many connections at once * Fix errors not propagating through web socket close Since the server closed the socket the caller has no chance to close with the right code and reason. Also abnormal closure is not a valid close code. * Fix test flake in reading output Without waiting for the copy you can sometimes get "file already closed". I guess process.Wait must have some side effect.
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.
I tried to slot this in without disturbing the existing code too much although I think it came out a bit wonky in places because of that. For example I wonder if reconnect should be a separate message since it seems odd that you can specify the ID of one command then pass a different arguments which will be ignored if the command with that ID is already running. But having one message does make things convenient.