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?
otan
left a comment
otan
left a comment
There was a problem hiding this comment.
i don't check this repo often, sorry for the super slow reply (you'll need to ping me if its >24 business hours)
have a couple of cleanup suggestions for you
| } | ||
|
|
||
| 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.
| 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
| t.Fatalf("expected driver.ErrBadConn, got: %#v", err) | ||
| } | ||
| if !cn.getBad() { | ||
| if err := cn.err.get(); err == nil { |
| 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
2 participants
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?