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

database/sql: if a context is cancelled, then pass back that error #25829

Closed
adam-p opened this issue Jun 11, 2018 · 13 comments
Closed

database/sql: if a context is cancelled, then pass back that error #25829

adam-p opened this issue Jun 11, 2018 · 13 comments

Comments

@adam-p
Copy link

@adam-p adam-p commented Jun 11, 2018

What version of Go are you using (go version)?

1.10.1

Does this issue reproduce with the latest release?

Probably?

What operating system and processor architecture are you using (go env)?

linux amd64; running on AWS Elastic Container Service Fargate.

What did you do?

(This is mostly a copy-paste from a golang-sql post, as requested by @kardianos .)

It’s pretty common (for client reasons) for our API server to get context.Canceled errors when clients terminate their connections before server response processing is complete. We detect and treat these differently (as warnings rather than errors, in a logging sense), since they’re client triggered and don’t indicate something wrong with the running of the server. So that’s fine.

But… Every now and then the context getting canceled will first surface as a DB error, like: “QueryRowContext failed: sql: Rows are closed”. It seems like a sort of race condition as to where the canceled state is first detected.

This case looks unavoidable, but, as above, we’d still prefer to detect and downgrade it to a warning. So… Is there a robust way to do this?

The origin of the error suggests that there’s no unique error type for “rows are closed” to compare against. So, is the string used for the error considered stable enough to compare against to detect the error type?

Or maybe any DB error could fall into checking “are the rows closed?” But I don’t see an API for checking that (either in database/sql or pgx).

Or maybe any DB error could fall into checking if the context has been canceled (ctx.Err() == context.Canceled)? But that seems like it might mask real DB errors if the context was only coincidentally cancelled. Low probability of that occurring, though.

But is there a right way to do this?

Thanks in advance.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jun 11, 2018
@ianlancetaylor ianlancetaylor changed the title Dealing with "QueryRowContext failed: sql: Rows are closed" due to context canceled database/sql: dealing with "QueryRowContext failed: sql: Rows are closed" due to context canceled Jun 11, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 11, 2018

I'm not clear on what the bug is here. Perhaps @kardianos can clarify.

(Questions rather than bug reports normally go to a forum, not the issue tracker; see https://golang.org/wiki/Questions. But I see that @kardianos suggested posting there.)

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jun 11, 2018

The poster wants to be able to differentiate errors caused by context cancelations and other database errors. I don't think we always pass back the context error, esp with the closed rows.

I'll assign to me and look into.

@kardianos kardianos self-assigned this Jun 11, 2018
@changpingc
Copy link

@changpingc changpingc commented Jun 18, 2018

I think here's a reproduction for this error we've been seeing.

func TestContextCancelBeforeRowsScan(t *testing.T) {
	testDb, err := NewTestDatabase()
	if err != nil {
		t.Fatal(err)
	}
	defer testDb.Close()

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	rows, err := testDb.QueryContext(ctx, `select "foo"`)
	if err != nil {
		t.Fatal(err)
	}
	defer rows.Close()

	// When we cancel the context after rows.Next() returns true,
	// database/sql.(*Rows).initContextClose monitors the context
	// and closes rows asynchronously, and subsequent rows.Scan()
	// returns errors.New("sql: Rows are closed") instead of
	// context.Canceled.
	for rows.Next() {
		cancel()
		time.Sleep(1000 * time.Millisecond)

		var foo string
		err := rows.Scan(&foo)

		if err != context.Canceled {
			t.Fatalf("expecting context.Canceled from rows.Scan(), got %v", err)
		}
	}
	if err := rows.Err(); err != context.Canceled {
		t.Fatalf("expecting context.Canceled from rows.Err(), got %v", err)
	}
}
@dsymonds
Copy link
Member

@dsymonds dsymonds commented Jun 28, 2018

If you want to behave differently based on whether a context has ended, check ctx.Err() != nil at the caller. You can't rely on context.Canceled being the error returned, since (a) that would prevent the things you call from ever adding context, and (b) that's not the only error that can signal that a context is done (e.g. context.DeadlineExceeded, or any error defined by a custom context implementation).

@adam-p
Copy link
Author

@adam-p adam-p commented Jun 28, 2018

@dsymonds For my purposes, it's specifically context.Canceled that I care about -- that I want to downgrade to warning. If the server (or DB) is exceeding deadline, then that is a server error and I want it to be an error. If a client app or web page gets closed or navigated away from while the request is being processed (so context canceled), that's not a server error and I want to downgrade it.

(But I'm sure your advice will be useful to others who come looking for this stuff.)

Except...

You can't rely on context.Canceled being the error returned, since (a) that would prevent the things you call from ever adding context

Can you elaborate? I'm not sure I understand.

@dsymonds
Copy link
Member

@dsymonds dsymonds commented Jun 28, 2018

If a client provides an arbitrarily short deadline, you'll get context.DeadlineExceeded, but it's not the server's fault.

My point about the extra context is that it is common for Go errors to be returned up the stack with extra information added. That loses the error type and value identity. For instance, if I wrote a function like this:

func f(ctx context.Context) error {
  if err := g(ctx); err != nil {
    return fmt.Errorf("running g: %v", err)
  }
  ...
  if err := h(ctx); err != nil {
    return fmt.Errorf("running h: %v", err)
  }
  ...
}

Now if g or h fail because the context was canceled, the error returned from f won't be context.Canceled. If you insist that f only return the plain error from g or h then you lose the context of which of those functions was failing, for instance. Or have to overload the error handling in f to be quite different.

@changpingc
Copy link

@changpingc changpingc commented Jul 5, 2018

@dsymonds I agree I don't think it must return ctx.Err(). However, I would like to see the error to be consistent regardless of when the context is canceled (super short deadline or long deadline) during a query, not sometimes ctx.Err() and sometimes "Rows are closed". I believe the issue summary has additional concerns.

While crafting the test case, I remember among various cases I came up with, only one resulted in "Rows are closed" error instead of ctx.Err(). That leads me to think it's very tricky to foresee the need to check the context, as we'd see ctx.Err() coming back most of the time in testing.

"sql: Rows are closed" also does not indicate the cause of closing. It'd be nice if the error extends ctx.Err() like in your example. Something like go/grpc's statusError seems nice.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jul 5, 2018

@dsymonds I don't think you're wrong, but I think you might be missing the point.

@changpingc I appreciate the test case. I'm not 100% certain we can address the issue, but I'll see what can be reasonably be done. I agree that when possible, it would be good to have the error that caused the abort to be represented by the error returned.

I think the conversation about what to do with errors when they are returned is less relevant here.

@adam-p
Copy link
Author

@adam-p adam-p commented Jul 24, 2018

We now have now seen another DB error that seems to be caused by context canceled and we're downgrading:

// If the request context is canceled by the user just before a transactions commit is
// attempted, then the following error will occur. We'll treat this like the other
// context.Canceled downgrades.
if cause.Error() == "sql: Transaction has already been committed or rolled back" &&
	ctx.Err() == context.Canceled {
	...downgrade
}
@kardianos kardianos changed the title database/sql: dealing with "QueryRowContext failed: sql: Rows are closed" due to context canceled database/sql: if a context is cancelled, then pass back that error Oct 27, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 27, 2018

Change https://golang.org/cl/145204 mentions this issue: database/sql: prefer to return Rows.lasterr rather then a static error

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 27, 2018

@adam-p Are you able to review / test the CL https://go-review.googlesource.com/c/go/+/145204 ? I think it would solve your issue.

@gopherbot gopherbot closed this in cf6e423 Oct 29, 2018
@adam-p
Copy link
Author

@adam-p adam-p commented Oct 29, 2018

Getting the DB errors to manifest in my local machine is pretty difficult, so I can't really test it. I read the diff, but it's hard to intuit what rs.lasterr will be, as it seems to mostly come from the driver. (I'll try it out in our dev env after release, but that's too late to be very useful.)

Question: Do you want to set rs.lasterr to the err param in lasterrOrErrLocked()? (You're not. I just wonder if you want the param to become lasterr when there isn't already one.)

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 29, 2018

Question: Do you want to set rs.lasterr to the err param in lasterrOrErrLocked()? (You're not. I just wonder if you want the param to become lasterr when there isn't already one.)

Nope.

If you look at (*Rows) close(err error) , it takes an optional error which is set "why" it closed. This is set when the context cancels from ctx.Err(). This sets rs.lasterr. Then we check this err msg before return a generic error message (maybe there is a more specific error we can return).

@golang golang locked and limited conversation to collaborators Oct 29, 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
6 participants
You can’t perform that action at this time.