Skip to content

Ensure that SqliteStmt.closed property is guarded. #608

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

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

collinvandyck
Copy link

Because the closed property of the SQLiteRows's *SqliteStmt
was not guarded, it was causing an issue during context
cancellation.

When a statement is performing a query(), if it determines that
the context has been canceled, it will launch a goroutine that
closes the resulting driver.Rows if it's not already completed:

go-sqlite3/sqlite3.go

Lines 1784 to 1799 in be424d2

if ctxdone := ctx.Done(); ctxdone != nil {
go func(db *C.sqlite3) {
select {
case <-ctxdone:
select {
case <-rows.done:
default:
C.sqlite3_interrupt(db)
rows.Close()
}
case <-rows.done:
}
}(s.c.db)
}
return rows, nil

If the driver.Rows is not done (and the context has been canceled),
it will interrupt the connection and more importantly, perform
a rows.Close(). The method rows.Close() guards the closed bool with
a sync.Mutex to set it to true.

If a reader is reading from the SqliteRow, it will call Next()
and that performs this check:

go-sqlite3/sqlite3.go

Lines 1913 to 1919 in be424d2

// Next move cursor to next.
func (rc *SQLiteRows) Next(dest []driver.Value) error {
if rc.s.closed {
return io.EOF
}
rc.s.mu.Lock()
defer rc.s.mu.Unlock()

Because this is not guarded, a data race ensues, and this was
actually caught by the Go race detector recently.

I didn't include a test case here because the fix seemed
straightforward enough and because race conditions are hard
to test for. It's been verified in another program that this
fixes the issue. If tests should be provided I'm more than
happy to do so.

Because the closed property of the SQLiteRows's *SqliteStmt
was not guarded, it was causing an issue during context
cancellation.

https://github.com/segmentio/go-sqlite3/blob/be424d27acde822f080bdcd8a7ae6abd4d7d801e/sqlite3.go#L1785-L1796

When a statement is performing a query(), if it determines that
the context has been canceled, it will launch a goroutine that
closes the resulting driver.Rows if it's not already completed.

If the driver.Rows is not done (and the context has been canceled),
it will interrupt the connection and more importantly, perform
a rows.Close(). The method rows.Close() guards the closed bool with
a sync.Mutex to set it to true.

If a reader is reading from the SqliteRow, it will call Next()
and that performs this check:

https://github.com/segmentio/go-sqlite3/blob/be424d27acde822f080bdcd8a7ae6abd4d7d801e/sqlite3.go#L1915-L1917

Because this is not guarded, a data race ensues, and this was
actually caught by the Go race detector recently.

I didn't include a test case here because the fix seemed
straightforward enough and because race conditions are hard
to test for.  It's been verified in another program that this
fixes the issue.  If tests should be provided I'm more than
happy to do so.
@coveralls
Copy link

coveralls commented Jul 18, 2018

Coverage Status

Coverage remained the same at 57.943% when pulling 4adeee5 on segmentio:fix-race-upstream into 3aefd9f on mattn:master.

@gjrtimmer
Copy link
Collaborator

LGTM

@gjrtimmer
Copy link
Collaborator

Verified already fixed in v2.0.0

@gjrtimmer gjrtimmer merged commit b3511bf into mattn:master Jul 19, 2018
@erikdw erikdw deleted the fix-race-upstream branch March 4, 2024 21:48
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

Successfully merging this pull request may close these issues.

3 participants