Implement reconnections with screen #25
Conversation
Going to add an alternate screen test next.
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
force-pushed
the
screen
branch
6 times, most recently
from
a5c5576
to
1d8cc30
Compare
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.
This makes it easier to test reconnects manually.
This way you do not need a subsequent resize and we can have the right size from the get-go.
code-asher
force-pushed
the
screen
branch
6 times, most recently
from
cfd4e72
to
1156b06
Compare
I think this helps make the tests a bit more concise.
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.
Without waiting for the copy you can sometimes get "file already closed". I guess process.Wait must have some side effect.
|
Without screen it will run like it did way back when without any reconnection logic (so disconnecting will lose your session). |
| s = NewSession(id, command, execer, options) | ||
| srv.sessions.Store(id, s) | ||
| go func() { // Remove the session from the map once it closes. | ||
| defer srv.sessions.Delete(id) |
There was a problem hiding this comment.
The delete happens outside the context of holding the mutex, since it is on a different goroutine.
There was a problem hiding this comment.
True, I was not sure if locking around the delete would change much functionally since sync.Map is threadsafe so I left it out. Vaguely I was thinking if loading and deleting happens at the same time whether it blocks on the mutex we add or the mutex internal to sync.Map it would be more or less equivalent.
I think there might be a gap in my logic though. And it does make sense to be explicit even if not.
code-asher
force-pushed
the
screen
branch
9 times, most recently
from
8975338
to
2c97aa4
Compare
|
I am down to move the Nix stuff into another PR. It is a decent way to get all the required system dependencies at the right versions for a project without having to install them by hand or edit your image (in this case, |
This resolves issues with unexpected "junk" output when reconnecting.
It might be easiest to review by commit, especially after the second one that reverts the original implementation.
I added a Nix flake to provide screen for testing.
See #24 for more details.