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

Racy context cancellation #745

Closed
azavorotnii opened this issue Sep 6, 2019 · 3 comments
Closed

Racy context cancellation #745

azavorotnii opened this issue Sep 6, 2019 · 3 comments

Comments

@azavorotnii
Copy link
Contributor

Current cancellation approach have couple racy cases. I propose PR to fix them:
#744

Issues details.

  1. Context cancellation is performed in separate go-routine which can live after query execution completes (sqlite3.go:1852):

	if ctxdone := ctx.Done(); ctxdone != nil {
		go func(db *C.sqlite3) {
			select {
			case <-ctxdone:
				select {
				case <-rows.done:
				default:
// <-- here query can complete and next query can start execution 
					C.sqlite3_interrupt(db)
					rows.Close() // < -- see case 2
				}
			case <-rows.done:
			}
		}(s.c.db)

This is rare case but it happen in our production environment sometimes. It causes next query executed on same connection to get "interrupted" error when its context was not cancelled.

See TestQueryRowContextCancelParallel in PR, it should reproduces issue almost always.


  1. Closing rows after interruption can cause QueryRowContext() to return sql.ErrNoRows, as (sql.go:3019):
func (r *Row) Scan(dest ...interface{}) error {
...
	if !r.rows.Next() {
		if err := r.rows.Err(); err != nil {
			return err
		}
		return ErrNoRows
	}

Because we close rows ourselves, rows.Next() returns io.EOF which is used by "sql" package both for rows.Next() and rows.Err() results.

See TestQueryRowContextCancel in PR.

@rittneje
Copy link
Collaborator

rittneje commented Sep 6, 2019

Note that sqlite3_interrupt was intended for a GUI and does not have the ability to actually cancel a particular query (see #648). One way this can cause a problem is if the context expires before the goroutine running execSync even starts. In this situation, sqlite3_interrupt will be a no-op, and sqlite3_step will run to completion, no matter how long it takes.

In short, there are still races with sqlite3_interrupt even with your fix, so please be careful how you use context.Context with this library. But your fix certainly seems reasonable.

@azavorotnii
Copy link
Contributor Author

azavorotnii commented Sep 6, 2019

One way this can cause a problem is if the context expires before the goroutine running execSync even starts

Good point, will put comment about it. (DONE).

@azavorotnii
Copy link
Contributor Author

@mattn, @gjrtimmer do you still maintain this repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants