feat: adds callback call when connection ends #4
Conversation
There was a problem hiding this comment.
I think the purpose of ConnectionFailedCallback is different from what we're trying to do here and repurposing it for established connection errors seems like it could lead to confusion.
Is there a reason we wouldn't want to handle this in agentssh instead? (E.g. get sshConn via ContextKeyConn).
Or perhaps ultimately, a new kind of callback named ConnectionErrorCallback could be added.
server.go
Outdated
| if srv.ConnectionFailedCallback != nil { | ||
| go func() { | ||
| wErr := sshConn.Wait() | ||
| srv.ConnectionFailedCallback(conn, wErr) |
There was a problem hiding this comment.
Instead of spawning a goroutine that can theoretically be left running after the server has closed, we could move this check to after the for loop for ch := range chans { ... }. That channel will be closed anyway before .Wait() here returns.
There was a problem hiding this comment.
Or perhaps ultimately, a new kind of callback named ConnectionErrorCallback could be added.
As I have spent some time recently diving in coder/ssh, I would recommend researching that approach.
BTW Sometimes the connection doesn't end, but will timeout eventually if there is no network activity (connection read/write timeout). With TCP keep alive enabled it might be hard to determine if the connection ended or if it is a transient network issue.
I do want to handle the logging in agentssh, but there are currently no callbacks at the scope I need. I want to log the connection lifetime & errors, so doing it in a session handler is the wrong scope: a connection could have 0, 1, or several sessions, so we can't easily arrange to log exactly once. There is a "connection" callback, but that has a |
I like this idea. I'll refactor to introduce a new callback. |
Signed-off-by: Spike Curtis <spike@coder.com>
3aef9e1 to
c92d705
Compare
|
I went with |
I want to add logging to the Coder Agent when ssh connections disconnect, including the reason for disconnection. c.f. coder/coder#7961
Currently, our ssh library doesn't expose or log this information, as it only calls the ConnectionFailedCallback if the connection fails during the initial SSH handshake, not if it fails later.
This PR makes it so that if the initial ssh handshake succeeds, then we start a goroutine that waits for the connection to end, and calls the callback. We already log calls to this callback in our server code, but in a separate PR we might want to adjust so that we only log at Info or Debug for "expected" disconnection errors like EOF.