Skip to content

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 10 commits into from
Apr 29, 2022
Merged

Add reconnecting ptys #23

merged 10 commits into from
Apr 29, 2022

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Apr 27, 2022

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.

@code-asher code-asher requested a review from kylecarbs April 27, 2022 06:11
@code-asher code-asher marked this pull request as ready for review April 27, 2022 18:46
smallnest/ringbuffer requires you to manually reset.
@code-asher
Copy link
Member Author

I got this up in my v1 deployment and it seems to be working!

@code-asher code-asher force-pushed the attach branch 2 times, most recently from 9743693 to a862b70 Compare April 28, 2022 21:18
Copy link
Member

@kylecarbs kylecarbs left a 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 code-asher merged commit 9120171 into coder:master Apr 29, 2022
@code-asher code-asher deleted the attach branch April 29, 2022 23:34
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants