issue 1062: Keep track of (context cancelled) error on connection, and make rows.Next return it #1064
Conversation
Instead of just whether the connection is ErrBadConn. Often times, the error
will still be ErrBadConn. But for expired/cancelled contexts, it will be the
error for the context. Most functions still return ErrBadConn per the
database/sql/driver contract ("ErrBadConn should only be returned from [...] a
query method"). For rows.Next() we return the context-related error.
The database/sql/driver contract doesn't look very precise. Is Next a "query
method" and should database/sql handle ErrBadConns when Next returns them?
Do we have more functions that should return the canceled-context error
message?
| @@ -166,6 +167,40 @@ type conn struct { | |||
| gss GSS | |||
| } | |||
|
|
|||
| type syncErr struct { | |||
| err error | |||
There was a problem hiding this comment.
any reason not to use type syncErr atomic.Value here, avoiding the mutex in the impls below?
There was a problem hiding this comment.
IIRC, I tried and ran into not being able to store a nil value. I currently don't see the code storing nil though, so I could have run into it while testing only. I'll give it a try.
There was a problem hiding this comment.
ah, the actual reason is that syncErr.set only wants to set an error if it is currently nil. atomic.Value's CompareAndSwap was introduced only in go1.17.
conn_test.go
Outdated
| func() { | ||
| defer cn.errRecover(&err) | ||
| panic(io.EOF) | ||
| }() | ||
| if err != driver.ErrBadConn { | ||
| t.Fatalf("expected driver.ErrBadConn, got: %#v", err) | ||
| } | ||
| if !cn.getBad() { | ||
| if err := cn.err.get(); err == nil { |
There was a problem hiding this comment.
we should assert what kind of error gets returned here
conn_test.go
Outdated
| @@ -721,7 +716,7 @@ func TestBadConn(t *testing.T) { | |||
| if err != driver.ErrBadConn { | |||
| t.Fatalf("expected driver.ErrBadConn, got: %#v", err) | |||
| } | |||
| if !cn.getBad() { | |||
| if err := cn.err.get(); err == nil { | |||
copy.go
Outdated
|
|
||
| closed bool | ||
|
|
||
| sync.Mutex // guards err | ||
| sync.Mutex // guards err and Result |
There was a problem hiding this comment.
whilst you're here, can you make this easier to not make a mistake by going:
type copyin struct {
// ....
type mu struct {
sync.Mutex
driver.Result
err error
}
}
then use
var c copyin
c.mu.Lock() ....
c.mu.Result() ...
that way it's clear how the mutex is used.
There was a problem hiding this comment.
i added the mu struct. by making the err & Result available under a different name, and fixing the uses, i noticed the usage wasn't correct.
|
looks like this is also failing tests |
|
The failed error was due to existing checks around "context canceled" not handling the error. I'll also look into your other cleanup suggestions. Hopefully within the next few days. |
feedback from otan
…unsafe usage. there may be unsafeness in this file. feedback from otan
For #1062
Instead of just whether the connection is ErrBadConn. Often times, the error
will still be ErrBadConn. But for expired/cancelled contexts, it will be the
error for the context. Most functions still return ErrBadConn per the
database/sql/driver contract ("ErrBadConn should only be returned from [...] a
query method"). For rows.Next() we return the context-related error.
The database/sql/driver contract doesn't look very precise. Is Next a "query
method" and should database/sql handle ErrBadConns when Next returns them?
Do we have more functions that should return the canceled-context error
message?