Skip to content

Commit

Permalink
database/sql: fix regression from earlier RawBytes fix
Browse files Browse the repository at this point in the history
The earlier CL 497675 for golang#60304 introduced a behavior change
that, while not strictly a bug, caused a bunch of test failures
in a large codebase. Rather than add behavior changes in a 10 year
old package, revert to the old behavior: a context cancelation
between Rows.Next reporting false and a call to Rows.Err should
not result in Rows.Err returning the context error.

That behavior was accidentally added in CL 497675 as part of changing
how contexts and Rows iteration worked.

Updates golang#60304
Updates golang#53970

Change-Id: I22f8a6a6b0b5a94b430576cf50e015efd01ec652
  • Loading branch information
bradfitz committed May 25, 2023
1 parent da578c1 commit 480a0c3
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 2 deletions.
19 changes: 17 additions & 2 deletions src/database/sql/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -2916,6 +2916,12 @@ type Rows struct {
// It is only used by Scan, Next, and NextResultSet which are expected
// not to be called concurrently.
closemuScanHold bool

// hitEOF is whether Next hit the end of the rows without
// encountering an error. It's set in Next before
// returning. It's only used by Next and Err which are
// expected not to be called concurrently.
hitEOF bool
}

// lasterrOrErrLocked returns either lasterr or the provided err.
Expand Down Expand Up @@ -2985,6 +2991,9 @@ func (rs *Rows) Next() bool {
if doClose {
rs.Close()
}
if doClose && !ok {
rs.hitEOF = true
}
return ok
}

Expand Down Expand Up @@ -3073,8 +3082,14 @@ func (rs *Rows) NextResultSet() bool {
// Err returns the error, if any, that was encountered during iteration.
// Err may be called after an explicit or implicit Close.
func (rs *Rows) Err() error {
if errp := rs.contextDone.Load(); errp != nil {
return *errp
// Return any context error that might've happened during row iteration,
// but only if we haven't reported the final Next() = false after rows
// are done, in which case the user might've canceled their own context
// before calling Rows.Err.
if !rs.hitEOF {
if errp := rs.contextDone.Load(); errp != nil {
return *errp
}
}

rs.closemu.RLock()
Expand Down
19 changes: 19 additions & 0 deletions src/database/sql/sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4443,6 +4443,25 @@ func TestContextCancelDuringRawBytesScan(t *testing.T) {
}
}

func TestContextCancelBetweenNextAndErr(t *testing.T) {
db := newTestDB(t, "people")
defer closeDB(t, db)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

r, err := db.QueryContext(ctx, "SELECT|people|name|")
if err != nil {
t.Fatal(err)
}
for r.Next() {
}
cancel() // wake up the awaitDone goroutine
time.Sleep(10 * time.Millisecond) // increase odds of seeing failure
if err := r.Err(); err != nil {
t.Fatal(err)
}
}

// badConn implements a bad driver.Conn, for TestBadDriver.
// The Exec method panics.
type badConn struct{}
Expand Down

0 comments on commit 480a0c3

Please sign in to comment.