Add Wait method to detect underlying SFTP connection closed #279
Conversation
|
|
||
| // Wait blocks until the conn has shut down, and return the error | ||
| // causing the shutdown. | ||
| func (c *clientConn) Wait() error { |
There was a problem hiding this comment.
What happens if two goroutines are calling Wait() at the same time?
There was a problem hiding this comment.
The one will be exited, the another one will be block forever when SFTP connection closed.
The Wait method should be called once.
There was a problem hiding this comment.
If we need Wait method to support called multiple times, may be implement it like ssh Wait method.
https://github.com/golang/crypto/blob/master/ssh/mux.go#L106-L113
There was a problem hiding this comment.
My intent was to provoke you to think about how this will be used, and what contracts need to be documented and what behavior people would expect from the library.
If we write this such that it may be called once and only once, we need to resolve how we would handle it being called more than once:
- Document that it cannot be used this way, and then point people to the code comment documenting the behavior when it comes in as a support ticket.
- Return an error in the case where it is called more than once, document that it returns an error in this case, and then wait for the PR to come in, which adds the functionality of handling an arbitrary number of calls.
Meanwhile, as you note, ssh.Wait can handle being called multiple times, and if we're clearly mimicking that library, it would be reasonable for them to expect the same behavior here.
There was a problem hiding this comment.
The ssh.Wait() method does seem to behave more like @wutaizeng's version. If you call it twice the second call will block forever, even if an error was returned by the call. The documentation isn't clear about this behavior, but here's a quicky example that shows it.
https://gist.github.com/eikenb/d3a92a399cd20beb54f1173246f32e2f
There was a problem hiding this comment.
Sorry, it seems we were talking 2 different Wait calls. I was looking at the public API Session.Wait, while @wutaizeng was pointing to an internal one on the mux struct.
There was a problem hiding this comment.
I think it’s still basically relevant that ssh.Session.Wait blocks forever if called more than once.
But 😕 I find that confusing behavior. There are a lot of cases where one might want to wait on a connection and do something after there is a disconnect, and at least some of those I could imagine someone wanting to do in separate goroutines.
But then as well, the ssh package could change, This package does not fall under the stability promise of the Go language itself, so its API may be changed when pressing needs arise.
There was a problem hiding this comment.
I prefer the current PR's behavior to ssh.Wait's block forever on additional read behavior and I don't see any value from modeling ssh's behavior here.
…ltiple goroutines
client.go
Outdated
| @@ -122,7 +123,7 @@ func NewClientPipe(rd io.Reader, wr io.WriteCloser, opts ...ClientOption) (*Clie | |||
| WriteCloser: wr, | |||
| }, | |||
| inflight: make(map[uint32]chan<- result), | |||
| errCh: make(chan error, 1), | |||
| errCond: sync.NewCond(new(sync.Mutex)), | |||
There was a problem hiding this comment.
Since this triggers at most only once, rather than a sync.Cond you can also use the feature that a receive on a closed channel immediately sends the zero value.
So, here you would have a closed: make(chan struct{}), where you have the whole c.errCond.Wait() block, all of that can be just <-c.closed; return c.err. Then instead of Broadcast you just have close(c.closed).
This tends to be a much more simple way to get a single-use state-change trigger of a broadcast unlock. While sync.Cond is better if it needs to broadcast repeated state-changes.
conn.go
Outdated
| return <-c.errCh | ||
| c.errCond.L.Lock() | ||
| defer c.errCond.L.Unlock() | ||
| for c.err == nil { |
There was a problem hiding this comment.
Now that I think about it, this Wait should probably return
8000
if the connection is shut down properly with a Close as well, right? In that case, we would want c.err == nil.
conn.go
Outdated
| c.errCh <- err | ||
| c.errCond.L.Lock() | ||
| c.err = err | ||
| c.errCond.Broadcast() |
There was a problem hiding this comment.
It would also probably make sense to have the c.err and boadcasting of a signal that it is now set in the broadcastErr method?
|
Looks good on my side. |
|
@eikenb Any plans to merge this PR ? |
|
Sorry for the delay. Personal life went sideways and I'm only now finding time to get back to this. I'll try to review (and hopefully merge) this soon. |
|
I'd like to see a test for Wait but I think I'll go ahead and merge this as it's been so long and it is such a simple feature. We can write up a test later. |
Closes #278