Add context methods #535

maddyblue · 2016-11-07T06:10:10Z

Progress on #506.

This change is

rikonor · 2016-12-22T18:10:15Z

Is this WIP?

maddyblue · 2016-12-22T18:48:23Z

Nope. Just waiting for reviews.

On Thu, Dec 22, 2016, 11:10 AM Or Rikon ***@***.***> wrote: Is this WIP? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACg3XP9FQsyLYy6R3t1TdE4uQQGAKSqks5rKr0HgaJpZM4Kq3lv> .

maddyblue · 2016-12-22T21:52:59Z

A cancelled context will prevent new queries and halt currently running queries.

…

On Thu, Dec 22, 2016 at 1:27 PM Or Rikon ***@***.***> wrote: @mjibson <https://github.com/mjibson> I see. Another question - assuming I provide a context with timeout, will an ongoing query be cancelled if the timeout is exceeded? Or is it only for queries that are currently queued for execution? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACg3cDvMRjfZpZmLeafLTgCoVm3Ounjks5rKt1AgaJpZM4Kq3lv> .

tamird · 2016-12-22T21:57:50Z

To be clear, it won't cancel the queries on the server, but it will unblock the client.

…

On Dec 22, 2016 16:53, "Matt Jibson" ***@***.***> wrote: A cancelled context will prevent new queries and halt currently running queries. On Thu, Dec 22, 2016 at 1:27 PM Or Rikon ***@***.***> wrote: > @mjibson <https://github.com/mjibson> I see. > > Another question - assuming I provide a context with timeout, will an > ongoing query be cancelled if the timeout is exceeded? Or is it only for > queries that are currently queued for execution? > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#535 (comment)>, or mute the > thread > <https://github.com/notifications/unsubscribe-auth/ AACg3cDvMRjfZpZmLeafLTgCoVm3Ounjks5rKt1AgaJpZM4Kq3lv> > . > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPNLGtftT6esWtPjvSseAnY1LaUvdks5rKvE9gaJpZM4Kq3lv> .

jhorowitz · 2016-12-22T22:03:28Z

Is there a reason not to add something which would send a cancel backend signal to postgres? Unless I'm misunderstanding (which I probably am), you seem to already have the PID. From there, canceling seems pretty simple. As long as you're a superuser, you could call pg_cancel_backend or pg_terminate_backend.

maddyblue · 2016-12-22T22:03:39Z

No, it does indeed cancel queries on the server. See the conn.Cancel function in conn.go. It opens a new connection instructing the server to cancel the original connection. See https://www.postgresql.org/docs/current/static/protocol-flow.html#AEN112861

tamird · 2016-12-22T22:12:06Z

Ah, I was recalling my experiences with MySQL, disregard.

…

On Dec 22, 2016 17:03, "Matt Jibson" ***@***.***> wrote: No, it does indeed cancel queries on the server. See the conn.Cancel function in conn.go. It opens a new connection instructing the server to cancel the original connection. See https://www.postgresql.org/ docs/current/static/protocol-flow.html#AEN112861 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPEbg4zIvWVu8Otk0aVgJwHOuMcucks5rKvO8gaJpZM4Kq3lv> .

kr · 2017-01-27T00:39:13Z

conn.go

@@ -22,6 22,8 @@ import (
 "time"
 "unicode"

+ "golang.org/x/net/context"


Is this a new dependency? If we're only using this for the interface, it would also be possible to copy just the interface definition into this package, and then avoid depending on golang.org/x/net/context.

We do use it for more than just the interface: see the creations of context.Background(). However since those are empty, it seems not horrible to copy both the interface definition as you describe and the implementation for context.Background into our own file here and just use those. That would allow for not having this dependency. I've made that change.

kr · 2017-01-27T00:42:50Z

conn.go

+ w.int32(cn.secretKey)
+
+ can.sendStartupPacket(w)
+ _ = can.c.Close()


It seems wasteful to open a connection just to send one command, then immediately tear down the whole connection. Is there some way to save this conn for some time so it can potentially be used by the pool?

Nope! The protocol for cancellation is to open a new connection that will immediately be closed.

kr · 2017-01-27T00:52:30Z

conn.go

+ }
+ }
+
+ if r != nil && ctx.Done() != context.Background().Done() {


It looks like this is trying to avoid the overhead of an extra goroutine in the case where the context will never be canceled. It would be better to write it as:

if r != nil && ctx.Done() != nil

Same for the other places that do this.

I remember reading some stuff from that it is bad form to do ctx.Done() != nil which is why it was done this way. But I do prefer the way you describe. Changed.

kr · 2017-01-27T00:52:40Z

conn.go

+ case <-closed:
+ }
+ }()
+ r.close = closed


Maybe consider pulling this whole blob of code out into a function? It is a bit subtle, and it's repeated several times. Maybe something like:

if r != nil && ctx.Done() != nil { r.close = watchCancel(ctx, cn.Cancel) }

with

func watchCancel(ctx context.Context, cancel func()) chan<- struct{} { closed := make(chan struct{}) go func() { select { case <-ctx.Done(): cancel() case <-closed: } }() return closed }

maddyblue

RFAL.

Comments about the new context.go would be appreciated. Not sure if it's a good idea, but it does remove the new dependency.

maddyblue · 2017-01-27T02:01:38Z

conn.go

+ }
+ }
+
+ if r != nil && ctx.Done() != context.Background().Done() {


I remember reading some stuff from that it is bad form to do ctx.Done() != nil which is why it was done this way. But I do prefer the way you describe. Changed.

maddyblue · 2017-01-27T02:11:05Z

conn.go

@@ -22,6 22,8 @@ import (
 "time"
 "unicode"

+ "golang.org/x/net/context"


We do use it for more than just the interface: see the creations of context.Background(). However since those are empty, it seems not horrible to copy both the interface definition as you describe and the implementation for context.Background into our own file here and just use those. That would allow for not having this dependency. I've made that change.

maddyblue · 2017-01-27T02:15:37Z

conn.go

+ case <-closed:
+ }
+ }()
+ r.close = closed


cbandy · 2017-01-27T07:04:43Z

context.go

+ Value(key interface{}) interface{}
+}
+
+type emptyCtx int


As I recall, an empty struct is actually the smallest value to have lying around.

You are correct, but in order to be a more direct copy of the context code I kept it as an int: https://tip.golang.org/pkg/context/?m=all#emptyCtx

cbandy · 2017-01-27T07:23:49Z

conn.go

+ can := &conn{}
+ can.c, err = dial(cn.dialer, cn.opts)
+ if err != nil {
+ return


Is there really nothing to be done here? Retry doesn't seem appropriate...

Is this method exported for the driver package? I don't see it in the docs.

I'm not sure what else we would do. The cancellation protocol itself says that the request may or may not be successful. And it's unclear what to do in the face of network error. It doesn't seem like an error here should set an error on cn either. This is currently my best idea.

I've unexported this method.

tamird · 2017-01-30T16:15:43Z

Why do we need this "contextInterface"? If we could move all the context-related functionality under the 1.8 build tag, we could just use "context" instead.

Seems like we can achieve that by having an embedded structure in conn which is empty when !go.18.

Reviewed 3 of 4 files at r2.
Review status: 3 of 4 files reviewed at latest revision, 4 unresolved discussions.

conn_go18.go, line 32 at r2 (raw file):

func (cn *conn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {
	if opts.Isolation != 0 {
		return nil, errors.New("isolation levels not supported")

seems like we can support both this and the below, but i guess that's not required for this PR.

go18_test.go, line 81 at r2 (raw file):

	// Delay execution for just a bit until db.ExecContext has begun.
	go cancel()

seems inherently flaky.

go18_test.go, line 103 at r2 (raw file):

	ctx, cancel := context.WithCancel(context.Background())
	tx, err := db.BeginTx(ctx, nil)

you need to clean up this tx to avoid goroutine leaks, I think.

go18_test.go, line 109 at r2 (raw file):

	// Delay execution for just a bit until tx.Exec has begun.
	go cancel()

ditto

Comments from Reviewable

maddyblue · 2017-01-31T20:54:29Z

I've moved all the context stuff to a go 1.8-only file. I like this design much more. It also fleshed out a bug.

Review status: 3 of 4 files reviewed at latest revision, 4 unresolved discussions.

conn_go18.go, line 32 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

seems like we can support both this and the below, but i guess that's not required for this PR.

another pr for sure

go18_test.go, line 81 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

seems inherently flaky.

I've added a 10ms sleep here. It's difficult to test this correctly.

go18_test.go, line 103 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you need to clean up this tx to avoid goroutine leaks, I think.

I added leaktest to this test and it passed without any cleanup. I don't think a Tx starts any goroutines.

go18_test.go, line 109 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

Comments from Reviewable

tamird · 2017-01-31T21:21:47Z

Let's squash this down, too.

Reviewed 3 of 4 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

conn.go, line 101 at r3 (raw file):

	scratch   [512]byte
	txnStatus transactionStatus
	txnClose  chan<- struct{}

txnClosed for consistency?

conn.go, line 871 at r3 (raw file):

	if len(args) == 0 {
		// ignore commandTag, our caller doesn't care
		res, _, err = cn.simpleExec(query)

unrelated change?

conn.go, line 889 at r3 (raw file):

		// used.
		st := cn.prepareTo(query, "")
		res, err = st.Exec(args)

also unrelated?

conn.go, line 1330 at r3 (raw file):

type rows struct {
	cn       *conn
	close    chan<- struct{}

consider calling this closed for consistency

conn_go18.go, line 18 at r3 (raw file):

	}
	var closed chan<- struct{}
	if ctx.Done() != nil {

move this below r, err := ... and then you can just set r.close directly instead of having closed in scope

go18_test.go, line 103 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I added leaktest to this test and it passed without any cleanup. I don't think a Tx starts any goroutines.

Evidence to the contrary: cockroachdb/cockroach@9bf770c

Cancelling the contexts in these tests is likely what's making it work.

Comments from Reviewable

maddyblue · 2017-01-31T21:34:53Z

Review status: all files reviewed at latest revision, 6 unresolved discussions.

conn.go, line 101 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

txnClosed for consistency?

Done.

conn.go, line 871 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

unrelated change?

yes, removed

conn.go, line 889 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

also unrelated?

removed

conn.go, line 1330 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider calling this closed for consistency

Done.

conn_go18.go, line 18 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

move this below r, err := ... and then you can just set r.close directly instead of having closed in scope

That's how it was before, but it's a bug. We need to start the cancellation listening before the first row is returned. If this is moved below the cn.query line, then only when we have a rows object are we able to cancel a query.

go18_test.go, line 103 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Evidence to the contrary: cockroachdb/cockroach@9bf770c

Cancelling the contexts in these tests is likely what's making it work.

Adding tx.Rollback() at the end produces an error: sql: Transaction has already been committed or rolled back. I'm not convinced adding a check to make sure tx.Rollback returns that error is useful. But I'm getting pretty convinced it doesn't have a leaky go routine if it already thinks it's been rolled back.

Comments from Reviewable

tamird · 2017-01-31T21:42:39Z

Can you add 1.8.x to the travis list? In fact, all the versions should end in .x, and tip is apparently now spelled master. https://docs.travis-ci.com/user/languages/go#Specifying-a-Go-version-to-use

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

conn_go18.go, line 18 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

That's how it was before, but it's a bug. We need to start the cancellation listening before the first row is returned. If this is moved below the cn.query line, then only when we have a rows object are we able to cancel a query.

Ah, thanks for explaining.

Comments from Reviewable

nathany · 2017-02-03T23:47:58Z

thanks! ref #531

maddyblue mentioned this pull request Dec 1, 2016

Go 1.8 support #549

Closed

kr reviewed Jan 27, 2017

View reviewed changes

maddyblue commented Jan 27, 2017

View reviewed changes

cbandy reviewed Jan 27, 2017

View reviewed changes

maddyblue mentioned this pull request Jan 28, 2017

pgwire: send BackendKeyData message for pgpool compatibility cockroachdb/cockroach#13009

Closed

support conntext cancellation

9c80e00

maddyblue merged commit a6657b2 into lib:master Feb 1, 2017

maddyblue deleted the ctx branch February 1, 2017 01:16

nathany mentioned this pull request Feb 3, 2017

Add Context Support #506

Closed

This was referenced Mar 19, 2017

Is there a way to detect and cancel a long-running query? #281

Closed

Hanging connections due to IO wait #584

Closed

pneisen mentioned this pull request Mar 25, 2017

This logic should be in the original driver Kount/pq-timeouts#1

Closed

cespare mentioned this pull request Mar 20, 2018

stmt should implement driver.StmtExecContext and driver.StmtQueryContext #732

Open

chappjc mentioned this pull request Aug 27, 2018

Use context for timeout/cancel in SQL driver decred/dcrdata#636

Closed

autrilla mentioned this pull request Mar 15, 2019

Queries for timed out requests are not cancelled mozilla/tls-observatory#387

Open

ethirolle mentioned this pull request Jan 14, 2021

Have cancelled HTTP Request Cancel Corresponding Database Queries grafana/grafana#22123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context methods #535

Add context methods #535

maddyblue commented Nov 7, 2016 •

edited

Loading

rikonor commented Dec 22, 2016

maddyblue commented Dec 22, 2016 via email

maddyblue commented Dec 22, 2016 via email

tamird commented Dec 22, 2016 via email

jhorowitz commented Dec 22, 2016

maddyblue commented Dec 22, 2016

tamird commented Dec 22, 2016 via email

kr Jan 27, 2017

maddyblue Jan 27, 2017

kr Jan 27, 2017

cbandy Jan 27, 2017

kr Jan 27, 2017

maddyblue Jan 27, 2017

kr Jan 27, 2017

maddyblue Jan 27, 2017

maddyblue left a comment

maddyblue Jan 27, 2017

maddyblue Jan 27, 2017

maddyblue Jan 27, 2017

cbandy Jan 27, 2017

maddyblue Jan 27, 2017

cbandy Jan 27, 2017

maddyblue Jan 27, 2017

tamird commented Jan 30, 2017

maddyblue commented Jan 31, 2017

tamird commented Jan 31, 2017

maddyblue commented Jan 31, 2017

tamird commented Jan 31, 2017

nathany commented Feb 3, 2017

Add context methods #535

Add context methods #535

Conversation

maddyblue commented Nov 7, 2016 • edited Loading

rikonor commented Dec 22, 2016

maddyblue commented Dec 22, 2016 via email

maddyblue commented Dec 22, 2016 via email

tamird commented Dec 22, 2016 via email

jhorowitz commented Dec 22, 2016

maddyblue commented Dec 22, 2016

tamird commented Dec 22, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddyblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird commented Jan 30, 2017

maddyblue commented Jan 31, 2017

tamird commented Jan 31, 2017

maddyblue commented Jan 31, 2017

tamird commented Jan 31, 2017

nathany commented Feb 3, 2017

maddyblue commented Nov 7, 2016 •

edited

Loading