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

Fix race condition in query cancellation #578

Merged
merged 1 commit into from Mar 13, 2017

Conversation

Projects
None yet
2 participants
@tamird
Collaborator

tamird commented Mar 13, 2017

It was previously possible for context cancellation to interleave with
query execution in such a way that the watcher goroutine would not be
scheduled until after the query which it was meant to watch was
complete, and would end up issueing a cancellation for the subsequent
query.

Fixes #577.


This change is Reviewable

@tamird tamird requested a review from mjibson Mar 13, 2017

Show outdated Hide outdated conn_go18.go
Show outdated Hide outdated conn_go18.go
Fix race condition in query cancellation
It was previously possible for context cancellation to interleave with
query execution in such a way that the watcher goroutine would not be
scheduled until after the query which it was meant to watch was
complete, and would end up issueing a cancellation for the subsequent
query.

Fixes #577.
can.sendStartupPacket(w)
_ = can.c.Close()
if err := can.sendStartupPacket(w); err != nil {

This comment has been minimized.

@mjibson

mjibson Mar 13, 2017

Collaborator

I don't see how the panics are caught here. cancel() is only invoked in a go routine, which means a defer'd recover in another go routine won't see it.

@mjibson

mjibson Mar 13, 2017

Collaborator

I don't see how the panics are caught here. cancel() is only invoked in a go routine, which means a defer'd recover in another go routine won't see it.

This comment has been minimized.

@tamird

tamird Mar 13, 2017

Collaborator

sendStartupPacket no longer panics, though.

@tamird

tamird Mar 13, 2017

Collaborator

sendStartupPacket no longer panics, though.

This comment has been minimized.

@mjibson

mjibson Mar 13, 2017

Collaborator

Ah, I didn't see that change cuz no reviewable.

@mjibson

mjibson Mar 13, 2017

Collaborator

Ah, I didn't see that change cuz no reviewable.

This comment has been minimized.

@tamird

tamird Mar 13, 2017

Collaborator

Yeah, GH reviews are terrible. Pro tip though: you can just navigate to https://reviewable.io/reviews/lib/pq/578.

@tamird

tamird Mar 13, 2017

Collaborator

Yeah, GH reviews are terrible. Pro tip though: you can just navigate to https://reviewable.io/reviews/lib/pq/578.

var err error
can := &conn{}
can.c, err = dial(cn.dialer, cn.opts)
func (cn *conn) cancel() error {

This comment has been minimized.

@mjibson

mjibson Mar 13, 2017

Collaborator

Why are we returning errors here at all? They are never used. In case we want them in the future and it's just good practice?

@mjibson

mjibson Mar 13, 2017

Collaborator

Why are we returning errors here at all? They are never used. In case we want them in the future and it's just good practice?

This comment has been minimized.

@tamird

tamird Mar 13, 2017

Collaborator

Yes, that was my thinking.

@tamird

tamird Mar 13, 2017

Collaborator

Yes, that was my thinking.

can.sendStartupPacket(w)
_ = can.c.Close()
if err := can.sendStartupPacket(w); err != nil {

This comment has been minimized.

@mjibson

mjibson Mar 13, 2017

Collaborator

Ah, I didn't see that change cuz no reviewable.

@mjibson

mjibson Mar 13, 2017

Collaborator

Ah, I didn't see that change cuz no reviewable.

@tamird tamird merged commit 472a074 into lib:master Mar 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tamird tamird deleted the tamird:context-cancel-race branch Mar 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment