Adjust Conn.QueryContext #212
Conversation
The change rewrites function code. Fixes #199
conn.go
Outdated
| case <-ctx.Done(): | ||
| // context has been cancelled or has expired, cancel the statement | ||
| if err := os.Cancel(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
If this line gets hit, nothing will ever read from rowsChan/errorChan - won't that leave the wrapQuery goroutine permanently blocked?
There was a problem hiding this comment.
wrapQuery method does not use channels anymore. It only calls ODBC APIs. If os.Cancel (which is also an ODBC API), did not unblock the ODBC API call execution, then I do not know what else can unblock it.
Perhaps I do not understand the code or your question.
There was a problem hiding this comment.
By "the wrapQuery goroutine" I meant "the goroutine that calls wrapQuery" - this chunk of code:
go func() {
err := c.wrapQuery(ctx, os, dargs)
if err != nil {
errorChan <- err
return
}
os.usedByRows = true
rowsChan <- &Rows{os: os}
}()Since errorChan and rowsChan are both unbuffered channels, unless something eventually closes or reads from them, this chunk of code could potentially block forever.
There was a problem hiding this comment.
Thanks for explaining. You are correct. I adjusted waitQuery code to read from errorChan and rowsChan channels. Hopefully this addresses your concern.
conn.go
Outdated
| select { | ||
| // ignore the error and return context.DeadlineExceeded instead | ||
| case <-errorChan: | ||
| return nil, context.DeadlineExceeded |
There was a problem hiding this comment.
Should this bubble ctx.Err()? The context may not have been canceled due to a deadline - a context.Context can be canceled several ways
| return nil, context.DeadlineExceeded | |
| return nil, ct.Err() |
There was a problem hiding this comment.
I had this in my original version of my code, but then this test failed here
Line 1953 in a2c1552
The error reported in the test was MSSQL driver specific. And I did not want to change the test.
Should I adjust the test instead?
There was a problem hiding this comment.
I think the test should be adjusted. Consider an HTTP handler:
func handleWebRequest(r *http.Request, w http.ResponseWriter) {
ctx := r.Context()
_, err := db.QueryContext(ctx, "select some_long_query")
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
// other http stuff
}Followed by something like
curl http://localhost/my_handler
# hit ctrl+c shortly after to cancel the requestI would expect the error there to be context.Canceled, or at least not context.DeadlineExceeded
There was a problem hiding this comment.
I used your original suggestion. Hopefully it is good enough.
|
Thank you for making this change! We have been stuck on an older version of this package for a really long time! |
conn.go
Outdated
| case <-ctx.Done(): | ||
| // context has been cancelled or has expired, cancel the statement | ||
| if err := os.Cancel(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Thanks for explaining. You are correct. I adjusted waitQuery code to read from errorChan and rowsChan channels. Hopefully this addresses your concern.
conn.go
Outdated
| select { | ||
| // ignore the error and return context.DeadlineExceeded instead | ||
| case <-errorChan: | ||
| return nil, context.DeadlineExceeded |
There was a problem hiding this comment.
I used your original suggestion. Hopefully it is good enough.
The change rewrites function code.
Fixes #199