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

proposal: database/sql: Rows.Scan should return lasterr if present #24431

Closed
erykwalder opened this issue Mar 16, 2018 · 9 comments
Closed

proposal: database/sql: Rows.Scan should return lasterr if present #24431

erykwalder opened this issue Mar 16, 2018 · 9 comments

Comments

@erykwalder
Copy link
Contributor

@erykwalder erykwalder commented Mar 16, 2018

When a context has been cancelled or timed out, Rows.Scan will return sql: Rows are closed. It would be nice to return the result of lasterr if it was not nil and not io.EOF, since an error was encountered between the call to Rows.Next and Rows.Scan. Additionally, for Row.Scan, there is no interface to detect if there was an error that occurred between Next and Scan, since only the results of Scan are exposed.

The caller could check context.Err() when it encounters sql: Rows are closed, but it would be more convenient for Scan to return the error value.

I believe it would be sufficient to add:

if rs.lasterr != nil && rs.lasterr != io.EOF {
	rs.closemu.RUnlock()
	return rs.lasterr
}

at the top of Scan, after:
rs.closemu.RLock()

@gopherbot gopherbot added this to the Proposal milestone Mar 16, 2018
@gopherbot gopherbot added the Proposal label Mar 16, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 17, 2018

I'll accept there is a documentation issue here at the least and there is possibly a reasonable code change..
The related issue is #24329
And comment in issue here: #23738 (comment)

Right now if rows.Scan produces an error, it isn't a good idea to necessarily just return that error and run. You'd really want to:

  1. Check Rows.Scan, assume err != nil.
  2. Check Rows.Err
  3. If Rows.Err == nil, call Rows.Close and return Rows.Scan's error.
  4. If Rows.Err != nil, return that error.

If Rows.Scan does not return an error, you may safely omit the Rows.Err check, but you shoud still check for an error from Rows.Close. Let's see what that would look like:

for rows.Next() {
    var id int64
    err := rows.Scan(&id)
    if err != nil {
        if rows.Err() != nil {
            return rows.Err()
        }
        rows.Close()
        return err
    }
}
if err := rows.Close(); err != nil {
    return err
}
// Do work.

or

var scanErr error
for rows.Next() {
    var id int64
    scanErr = rows.Scan(&id)
    if scanErr != nil {
        break
    }
}
if rows.Err() != nil {
    return rows.Err()
}
if err := rows.Close(); err != nil {
    return err
}
if scanErr != nil {
    return scanErr
}
// Do work.

With your proposal you could write:

for rows.Next() {
    var id int64
    err := rows.Scan(&id)
    if err != nil {
        rows.Close() // We may be able to ignore this error.
        return err // This would be either a scan error or Rows.Err(lasterr) from Rows.Next.
    }
}
if err := rows.Close(); err != nil {
    return err
}
// Do work.

It does seem like it could simplify some code, or make existing code actually more useful. The downside someone may be (probably is) checking the text of rows.Scan's error and doing something with it. So this may be a compatibility issue.

(Note, I would only be concerned about the close error if I was writing to the database while also reading from it.)

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Mar 17, 2018

I'd assume that calling Scan would always return a syntax/parsing/type casting failure from reading the value, and I might rewrap in a different way than I'd wrap rows.Err or rows.Close, which I would expect to be a connection failure or timeout.

@erykwalder
Copy link
Contributor Author

@erykwalder erykwalder commented Mar 17, 2018

One of the motivations for this is that in the case of Row, there isn't an option to check Close or Err from the underlyingRows. To catch the context close error would require this (or something similar):

err := db.QueryRowContext(ctx, q).Scan(&id)
if err.Error() == "sql: Rows are closed" && ctx.Err() != nil {
    return ctx.Err()
} else if err != nil {
    return err
}

Which could just be simplified to:

err := db.QueryRowContext(ctx, q).Scan(&id)
if err != nil {
    return err
}

Rows Next->Scan->Err seems to have been built with a single-threaded use in mind, but with the additional of context timeouts and cancellations, other errors can happen between the successive calls. Scan already returns an error (sql: Rows are closed), but sometimes the error could be more specific or give the cause.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 25, 2018

@erykwalder Your last comment isn't convincing; Row.Scan checks for Rows.Err and Rows.Scan and Rows.Close errors all in one.

But I agree with you in general, I really see no reason not to return the lasterr if present on scan if Rows is closed.

@bradfitz I don't think this will be a compatibility problem, but theoretically someone could be checking the error text from rows.Scan. Do you have an opinion on this? If you don't I lean to doing this.

@erykwalder
Copy link
Contributor Author

@erykwalder erykwalder commented Mar 28, 2018

@erykwalder Your last comment isn't convincing; Row.Scan checks for Rows.Err and Rows.Scan and Rows.Close errors all in one.

It does, but again, does not handle errors that can happen asynchronously or from separate routines (specifically a context being timed out or cancelled). This is demonstrated by a simple program which runs inconsistently:

package main

import (
	"context"
	"database/sql"
	"fmt"
	"time"

	_ "github.com/lib/pq"
)

func main() {
	db, err := sql.Open("postgres", "postgres:///test?sslmode=disable")
	if err != nil {
		panic(err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Microsecond)
	defer cancel()

	err = db.QueryRowContext(ctx, `SELECT 1`).Scan(new(int))
	if err != nil && err != context.DeadlineExceeded {
		fmt.Println(err)
	}
}

With sufficient runs, it will print sql: Rows are closed.

Looking at the source of Row.Scan, the problem is identifiable (comments are mine):

func (r *Row) Scan(dest ...interface{}) error {
	if r.err != nil {
		return r.err
	}

	defer r.rows.Close()
	for _, dp := range dest {
		if _, ok := dp.(*RawBytes); ok {
			return errors.New("sql: RawBytes isn't allowed on Row.Scan")
		}
	}

	if !r.rows.Next() {
		if err := r.rows.Err(); err != nil {
			return err
		}
		return ErrNoRows
	}
	// lastErr updated if context is closed by another routine here
	// won't be caught or returned
	err := r.rows.Scan(dest...)
	if err != nil {
		return err
	}
	if err := r.rows.Close(); err != nil {
		return err
	}

	return nil
}
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 2, 2018

@bradfitz I don't think this will be a compatibility problem, but theoretically someone could be checking the error text from rows.Scan. Do you have an opinion on this? If you don't I lean to doing this.

Let's try this and test it over the next 4 months and see.

I think making Scan return the correct errors on context cancels makes sense. Returning "Rows are closed" when they were really just canceled is slightly misleading, even if they were closed by the context-cancel-waiting goroutine.

@erykwalder
Copy link
Contributor Author

@erykwalder erykwalder commented Apr 2, 2018

@kardianos I'm happy to work on this if you don't already have something. I've already submitted a CLA fwiw.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Apr 2, 2018

@erykwalder That would be fine with me.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 3, 2018

Change https://golang.org/cl/104276 mentions this issue: database/sql: return context errors from Rows.Scan

@gopherbot gopherbot closed this in 16f32a0 Apr 11, 2018
@golang golang locked and limited conversation to collaborators Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.