Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: use screen for reconnecting terminal sessions if available#8640

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
code-asher merged 30 commits intomainfromasher/reconnection-with-screen
Aug 14, 2023

Conversation

code-asher
Copy link
Member

@code-ashercode-asher commentedJul 21, 2023
edited
Loading

Thisfixes#3666 and similar issues.

I copied much of this from v1 (coder/wsep#25) but one major difference is that I was asked to keep the existing behavior as a fallback so I broke out parts into an interface then moved the existing code into said interface.

I think having a fallback makes more sense than it did for v1 since we have macOS and Windows that are not using screen yet (I have not looked into whether screen can run on Windows yet and on macOS it works for me locally but in CI screen will sometimes refuse to find a session even if youscreen -list to verify that exact name does in fact exist).

@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch 5 times, most recently froma7468cd tobdd4a6aCompareJuly 21, 2023 02:49
@code-ashercode-asher changed the titlefix: use screen for reconnecting terminal sessionsfix: use screen for reconnecting terminal sessions if availableJul 21, 2023
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch 23 times, most recently from9ae2251 to59aceb9CompareJuly 26, 2023 21:16
In some cases it might get closed but it is not a guarantee like thecomment claims it is.While we could implement this behavior properly we already close theconnection in the caller so it seems unnecessary.
Still need to figure out a way to test both, but we would rather not addthe ability to select the backend through the API.
Since we might add to the map after it closes, tweak the delete to alsorun when the pty closes.I think this also fixes that issue where if you canceled the context youpass into Attach it would not do anything since readConnLoop does notuse the context for anything but logging.
We want to send the error from failing to wait for the version command,not the error from closing/killing the pty or process.
It is possible for the process to immediately exit (for example if yourun `echo hello`), then we would wait for the `version` command tosucceed but it never will because the session is already gone.If we immediately read to the process then we can tell when it has goneaway and we can abort.
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch fromb90c051 to7a8ec2eCompareAugust 10, 2023 01:43
Since we used the same var all I did with my other change was use thesame logger anyway.Now it will not have the connection ID, which is correct since thepty is not tied to a single connection.
Copy link
Contributor

@spikecurtisspikecurtis left a comment

Choose a reason for hiding this comment

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

LGTM.

I do think it would be an improvement to consolidate the bufferedReconnectingPTY down to a single lock, as I mentioned in a comment thread, but what you have now is fine.

I moved the conn closes back to the lifecycle, too.
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch from5f905f5 to9b88a68CompareAugust 11, 2023 00:23
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch from83ac118 to9fb4230CompareAugust 11, 2023 01:54
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch from9fb4230 tod4170caCompareAugust 11, 2023 01:54
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch 2 times, most recently froma83cb61 tob30bc20CompareAugust 11, 2023 18:43
We do not really do anything with it other than just return it, and ifwe do need to do something with it we can just do it on the return from`waitForStateOrContext`, we probably would not need to use it behind themutex.Also we should check the state outside since there is technically noguarantee an error is set on a state change (although in practice forclose/done there always is, maybe this should be enforced in some way).
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch fromb30bc20 to88a6b96CompareAugust 11, 2023 18:44
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch from802b2cc to748cb33CompareAugust 14, 2023 17:51
@code-ashercode-asherforce-pushed theasher/reconnection-with-screen branch from748cb33 to968526dCompareAugust 14, 2023 17:53
@code-ashercode-asher merged commitb993cab intomainAug 14, 2023
@code-ashercode-asher deleted the asher/reconnection-with-screen branchAugust 14, 2023 19:19
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 14, 2023
@code-asher
Copy link
MemberAuthor

For posterity's sake, I made a mistake because I forgot waiting on the cond releases the lock; hotfix can be found here:#9094

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@spikecurtisspikecurtisspikecurtis approved these changes

Assignees

@code-ashercode-asher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Junk is being inserted into the web terminal
2 participants
@code-asher@spikecurtis

[8]ページ先頭

©2009-2025 Movatter.jp