Close also closes user-provided SSH clients
#79
Comments
bramvdbogaerde
added a commit
that referenced
this issue
Mar 31, 2024
The issue was fixed in the way proposed in #79, an additional field called `CloseHandler` was added to the `Client` struct. This handler can either be `EmptyHandler` which is equivalent to a no-op, or a `CloseSSHClient` which closes the passed SSH client when executed. The `EmptyHandler` is used by default unless `Client.Connect()` is called to establish the SSH connection. The reasoning is that whenever someone passes their own `ssh.Client` to the library, `Connect()` will not be called, thus signaling that we should not manage the `ssh.Client`. To ensure the correctness of this fix, two additional test cases were added (1) `TestUserSuppliedSSHClientDoesNotClose` which creates an `scp` client using a existing `ssh.Client` and ensures that the client is not closed by attempting to create a new session from it. (2) `TestSSHClientNoLeak` which uses `Connect()` to establish the SSH connection and ensures that its underlying `ssh.Client` is no longer functioning using the same mechanism as (1).
jpdoyon
pushed a commit
to jpdoyon/go-scp
that referenced
this issue
Jun 28, 2024
The issue was fixed in the way proposed in bramvdbogaerde#79, an additional field called `CloseHandler` was added to the `Client` struct. This handler can either be `EmptyHandler` which is equivalent to a no-op, or a `CloseSSHClient` which closes the passed SSH client when executed. The `EmptyHandler` is used by default unless `Client.Connect()` is called to establish the SSH connection. The reasoning is that whenever someone passes their own `ssh.Client` to the library, `Connect()` will not be called, thus signaling that we should not manage the `ssh.Client`. To ensure the correctness of this fix, two additional test cases were added (1) `TestUserSuppliedSSHClientDoesNotClose` which creates an `scp` client using a existing `ssh.Client` and ensures that the client is not closed by attempting to create a new session from it. (2) `TestSSHClientNoLeak` which uses `Connect()` to establish the SSH connection and ensures that its underlying `ssh.Client` is no longer functioning using the same mechanism as (1).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The library allows users to pass their (already connected) SSH clients as the underlying transport for the
scpsession. For ease of use, the library can also create its own SSH client, and will close it when theClosemethod is called.Unfortunately, when used, it will also close the user-provided SSH client. A workaround to this problem is not to call
Closeat all, since it only closes the SSH client and any other resources are already freed. However this goes against common patterns such asdefer client.Close().As a permanent fix I propose to add a
closeHandlerfield to the client. This field should contain a function, or a struct with a single method that frees the resources that we manage. This handler is a no-op by default, but would close the ssh Client if a call toConnectwas made.The text was updated successfully, but these errors were encountered: