Skip to content

Adds a timeout context to the ssh waiter to prevent indefinite hanging#4635

Merged
reybard merged 2 commits intotrunkfrom
ssh-waiter-timeout
Nov 2, 2021
Merged

Adds a timeout context to the ssh waiter to prevent indefinite hanging#4635
reybard merged 2 commits intotrunkfrom
ssh-waiter-timeout

Conversation

@reybard
Copy link
Contributor

@reybard reybard commented Oct 28, 2021

This patch adds a timeout to the ssh call waiter in the liveshare client. While the hope is to never have issues that would cause the liveshare client call to hang in the first place, we have now seen cases of it happening so this prevents users from having requests stall for more than a couple of minutes.

I was able to test this works by having my local build ssh into a codespace that I had changed the sshd_config port on without restarting sshd itself. gh cs ssh then tries to ssh using the incorrect port and hangs for the correct 2 mins before giving:

» go run cmd/gh/main.go cs ssh                                                                                                                                                                                                                                                                                          1 ↵
? Choose codespace: github/github: fix-pkg-permission-expiry
kex_exchange_identification: read: Connection reset by peer
tunnel closed: error opening streaming channel for new connection: error getting stream id: context deadline exceeded
exit status 1

Without the new context the request hangs for much longer (reportedly 8-9 minutes at times) before aborting.

Closes: https://github.com/github/cli/issues/100

@reybard reybard requested a review from josebalius October 28, 2021 23:31
@reybard reybard requested a review from a team as a code owner October 28, 2021 23:31
Copy link
Contributor

@josebalius josebalius left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, I looked at all rpc calls and can't think why any one of them should take more than 2 minutes. StartSSHServer is the one with the potential to take the longest in the case that it has to install but if @edgonmsft said it should never take that long than 👍

Copy link
Contributor

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

I agree, looks sound.

Copy link

@Satansseed6 Satansseed6 left a comment

Choose a reason for hiding this comment

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

Approve

@reybard reybard merged commit 3a4d947 into trunk Nov 2, 2021
@reybard reybard deleted the ssh-waiter-timeout branch November 2, 2021 18:46
@VictorBatta VictorBatta mentioned this pull request Dec 4, 2021
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.

4 participants