Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug for sending cancel request to terminate a long running query #1952

22 changes: 12 additions & 10 deletions base.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,17 @@ func (db *baseDB) withConn(
if ctx != nil && ctx.Done() != nil {
fnDone = make(chan struct{})
go func() {
select {
case <-fnDone: // fn has finished, skip cancel
case <-ctx.Done():
err := db.cancelRequest(cn.ProcessID, cn.SecretKey)
if err != nil {
internal.Logger.Printf(ctx, "cancelRequest failed: %s", err)
for {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in a for loop? I looks like the previous logic would work. This appears to have this for loop run forever, as there is no way to break out of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point! My bad. Reverted the change.

select {
case <-fnDone: // fn has finished, skip cancel
case <-ctx.Done():
err := db.cancelRequest(cn.ProcessID, cn.SecretKey)
if err != nil {
internal.Logger.Printf(ctx, "cancelRequest failed: %s", err)
}
// Signal end of conn use.
fnDone <- struct{}{}
}
// Signal end of conn use.
fnDone <- struct{}{}
}
}()
}
Expand Down Expand Up @@ -188,8 +190,8 @@ func (db *baseDB) shouldRetry(err error) bool {
"53300", // too_many_connections
"55000": // attempted to delete invisible tuple
return true
case "57014": // statement_timeout
return db.opt.RetryStatementTimeout
case "57014": // query_canceled
return db.opt.RetryCancelledQuery
default:
return false
}
Expand Down
4 changes: 2 additions & 2 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ type Options struct {
// Maximum number of retries before giving up.
// Default is to not retry failed queries.
MaxRetries int
// Whether to retry queries cancelled because of statement_timeout.
RetryStatementTimeout bool
// Whether to retry queries cancelled because of statement_timeout or cancel request.
RetryCancelledQuery bool

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this would be a breaking change. I recommend we leave the name the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I will revert the change to keep as it is.

// Minimum backoff between each retry.
// Default is 250 milliseconds; -1 disables backoff.
MinRetryBackoff time.Duration
Expand Down