transport: fail NewStream() with better error message when conn is closed #4426
Conversation
|
Found when debugging |
|
The test you changed now appears to be flaky. |
| t.mu.Lock() | ||
| err := t.closeErr | ||
| t.mu.Unlock() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I don't believe the lock is necessary, but it's harmless in any case. t.cancel() is called only after t.closeErr is set, and t.closeErr can only be set once, before t.cancel().
There was a problem hiding this comment.
t.ctx is derived from a parent context, so t.cancel() isn't the only way to cancel t.ctx. And the read and write may race in the other case.
(That parent context is ClientConn.ctx, so this only happens when the ClientConn is closed)
There was a problem hiding this comment.
That makes sense, but is pretty complicated. How about:
func (t *http2Client) getCloseErr() {
t.mu.Lock()
defer t.mu.Unlock()
if err := t.closeErr; err != nil {
return err
}
if t.ctx.Err() != nil {
return ErrConnClosing
}
return nil // Not closed, why are you calling this??
}Or is there some way to block until t.Close finishes so we know t.closeErr is valid?
There was a problem hiding this comment.
How is this different?
And I don't like returning nil here. Even if it should never happen.
| t.mu.Lock() | ||
| err := t.closeErr | ||
| t.mu.Unlock() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
That makes sense, but is pretty complicated. How about:
func (t *http2Client) getCloseErr() {
t.mu.Lock()
defer t.mu.Unlock()
if err := t.closeErr; err != nil {
return err
}
if t.ctx.Err() != nil {
return ErrConnClosing
}
return nil // Not closed, why are you calling this??
}Or is there some way to block until t.Close finishes so we know t.closeErr is valid?
| // - if it is after the transport is closed (case <-ct.ctxDone), | ||
| // we don't care about the error. |
There was a problem hiding this comment.
Why would the transport be closed? It was gracefully closed, but we have an active stream (from line 755) so it should never close, should it? Unless the server ends the stream, but it looks like it does not, until it receives the client's end-stream.
There was a problem hiding this comment.
We start 200 goroutines to run this.
Some of them could be so slow that they run after the end of the line 755 stream.
menghanl
force-pushed
the
better_conn_close_error
branch
from
b122a1f
to
9602024
Compare
|
Tests are failing: |
|
This PR no longer works after moving
This will need more thoughts, and probably more changes. |
|
Can you file an issue to follow-up on this later? (Unless you have something you're actively working on already.) |
No description provided.