Skip to content
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

Client Timeout #211

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Client Timeout #211

merged 2 commits into from
Nov 10, 2023

Conversation

amtelekom
Copy link
Contributor

Implement timeouts for SSH clients during normal operation.

So far, this was only possible in servers.

Note that this doesn't include provisions for sending keepalives, so unless the server sends those faster than the chosen timeout, the application needs to track this itself for now.

@Eugeny Eugeny merged commit 43bdc07 into Eugeny:main Nov 10, 2023
4 checks passed
@Eugeny
Copy link
Owner

Eugeny commented Nov 10, 2023

Thank you!

@mmirate
Copy link
Contributor

mmirate commented Nov 10, 2023

Note that this doesn't include provisions for sending keepalives, so unless the server sends those faster than the chosen timeout, the application needs to track this itself for now.

@amtelekom, could you please explain the difference between what you're looking for regarding keepalives, and https://github.com/warp-tech/russh/pull/211/files#diff-a54f5b4b2c7b6955dd496e3f9b7bcc52dac6226eb3450c4785bd7957e47f0c1dR782-R785? (as added in #196)

@Eugeny
Copy link
Owner

Eugeny commented Nov 10, 2023

@mmirate this PR is confusingly named - it adds an automatic disconnect if no server message (keepalive or others) is received within inactivity_timeout

@mmirate
Copy link
Contributor

mmirate commented Nov 10, 2023

@Eugeny Ah, so given that in #196 the client-sent keepalives are hardcoded to want_reply = true, then I suppose that the author meant to say: the application is responsible for ensuring that its provided configuration satisfies keepalive_interval < inactivity_timeout, plus round-trip-time plus processing time plus uncertainty margin?

@amtelekom
Copy link
Contributor Author

the application is responsible for ensuring that its provided configuration satisfies keepalive_interval < inactivity_timeout, plus round-trip-time plus processing time plus uncertainty margin?

Yes - although I wasn't even aware of #196 because we've implemented this in our internal (downstream) fork for some time now, and I only got to rebase & upstream this a few days ago.

I fear that, now that I look at it more closely again, the sending of the keepalive will reset the timeout, thus breaking the client timeout as implemented in this PR when a client keepalive is configured.

I guess I'll move the timeout to manually updated sleep_untils (like the keepalives) instead of a duration stared with each tokio select! loop as soon as I get to it, then combining both settings should work as well.

@mmirate mmirate mentioned this pull request Nov 13, 2023
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.

3 participants