Repository navigation
Conversation
Neither doc comment said the two things a caller needs to know. They are null by default, so out of the box nothing stops a server that accepts the connection and then goes quiet from leaving the client waiting as long as the socket stays open. `keepAliveInterval` looks like it would cover that and does not, since it only starts once the connection is up. And they are deadlines over the whole phase rather than inactivity timers, with the callbacks inside the window: `onVerifyHostKey` is awaited inside the handshake one, `onPasswordRequest` and `onUserInfoRequest` inside the auth one. So a value chosen to bound an unresponsive server also cuts off a person reading a fingerprint or typing a password. The README recommended 15 seconds for both next to a literal password, which hides exactly that. No behaviour change. Setting a default was the obvious move and is the wrong one for the same reason: any value low enough to be a useful network bound is too low for a human.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
=======================================
Coverage 77.84% 77.84%
=======================================
Files 81 81
Lines 6395 6395
=======================================
Hits 4978 4978
Misses 1417 1417
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the review of what 4.1.0 could pick up. I went in to give these two a sensible default and came out documenting them instead, because a default is the wrong fix.
What is wrong today
Neither doc comment says the two things a caller needs.
They are null by default.
keepAliveIntervaldefaults to 10 seconds, which makes it look like connection liveness is handled, but it only starts once the connection is up. Out of the box nothing stops a server that accepts the TCP connection and then goes quiet from leaving the client waiting for as long as the socket stays open.They are deadlines over the whole phase, not inactivity timers, and your own callbacks run inside them.
onVerifyHostKeyis awaited inside the handshake window,onPasswordRequestandonUserInfoRequestinside the auth one. So any value chosen to bound an unresponsive server also cuts off a person reading a fingerprint or typing a password.The README made that worse rather than better: it recommended 15 seconds for both, next to
onPasswordRequest: () => '<password>'. A literal never takes 15 seconds and a real prompt often does.Why not just give them defaults
That was the plan and it does not work. Any value low enough to be a useful bound on a dead server is too low for a human at a prompt, because the same timer covers both. The honest fix is a bound on inactivity while waiting on the peer, which is a real design task rather than a default, and there is already a precedent for the distinction in this codebase:
SSHHttpClient.idleTimeoutfrom #230 is an inactivity timeout for exactly this reason.So this PR only makes the current behaviour legible. If you want the inactivity bound it is worth its own issue.
Contents
Doc comments on both fields, the README paragraph rewritten to say what the values should be picked against, and a changelog entry.
No behaviour change.
dart formatanddart analyze --fatal-infosclean, 703 tests passing.