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

Jump to bottom

database/sql: Use sql.Tx.ctx by default in sql.Tx methods instead of context.Background() #20098

Closed
bgaifullin opened this issue Apr 24, 2017 · 10 comments
Closed

database/sql: Use sql.Tx.ctx by default in sql.Tx methods instead of context.Background() #20098

bgaifullin opened this issue Apr 24, 2017 · 10 comments
Labels
FrozenDueToAge

Comments

@bgaifullin
Copy link
Contributor

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

go version go1.8 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/b.gaifullin/Sources/mail/target/cmake-build-debug/go:/Users/b.gaifullin/Sources/mail/target/go/target-cpp/_vendor"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qn/p8wx9pvd3dd90xmd6w_kx50c0000gn/T/go-build037257915=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I found that method of sql.Tx uses context.Background by default even sql.Tx has ctx property.
I expect that creating sql.Tx via method sql.DB.BeginTx, the sql.Tx will use the same context for all methods event another is not specified directly through special methods, which accept context argument.

for example:

func (tx *Tx) Prepare(query string) (*Stmt, error) {
	return tx.PrepareContext(tx.ctx, query)
}

// instead of 

func (tx *Tx) Prepare(query string) (*Stmt, error) {
	return tx.PrepareContext(context.Background(), query)
}
@matipan
Copy link

matipan commented Apr 24, 2017

Hi @bgaifullin, the subject of this issue doesn't have the correct format. It should be something like: database/sql: (problem or suggestion here)

@bgaifullin bgaifullin changed the title Use Tx.ctx by default in Tx methods instead of context.Background() database/sql: Use sql.Tx.ctx by default in sql.Tx methods instead of context.Background() Apr 24, 2017
@bgaifullin
Copy link
Contributor Author

Hi @matipan, I have updated title. thank you.

@kardianos
Copy link
Contributor

kardianos commented Apr 24, 2017 •
edited

@bgaifullin This suggestion has merit. However I would resist this suggestion for two reasons:

  1. It isn't strictly backwards compatible with previous behavior. If a person uses this with a query, bounding the life of the Tx with a ctx but using the old method db.Query(...) the query will now be canceled where as before it would complete first. This may or may not be a huge issue as the transaction would be rolled back right after.

  2. More pressing, we are already breaking some of the context design "rules" by storing the ctx. It also establishes the old non-context methods as useful. If at all possible code should always pass around context directly.

  3. I'd like to establish a normal set of methods to use as a standard interface, even if not explicitly specified. Those will generally be the Context methods now.

All that being said, in this case all queries in a Tx live in the span of the Tx. It does have some merit in this case. I'm going to hold off closing it for now, but it is unlikely to be accepted.

Thanking.

@bgaifullin
Copy link
Contributor Author

bgaifullin commented Apr 24, 2017 •
edited

@kardianos, thank you. My point was that the sql.Tx was created with context, and methods Commit and Rollback use this context. But other methods do not use and I as a user have to pass the same context to methods Query, Exec. IMHO it is unlogical.

@kardianos
Copy link
Contributor

@bgaifullin I understand your point. It has merit. Do you have a response for my two points?

@bgaifullin
Copy link
Contributor Author

  1. If user creates Tx with context, in case if context expires it will have error on Commit, the result will be same.
    2, 3: If there are be method like CommitContext and RollbackContext, it will be ok. but there is no such methods and there is no way to implement them without breaking backward compatibility with previous behaviour.

@gopherbot
Copy link

CL https://golang.org/cl/44956 mentions this issue.

@kardianos
Copy link
Contributor

I would love to see a proposal for adding CommitContext and RollbackContext. They can be added in a backwards compatible way, just like the other context methods.

@kardianos
Copy link
Contributor

@bgaifullin If there is a go2, there wouldn't be any query variants without a context argument. I want to strongly encurage all uses of Query and Exec methods to include a context directly in their arguments.

This is probably too late for a go1.9 change.

@rsc My sense for some of these topics (context, go2, release cycle) have been wrong a number of times in the past. Would you be willing to weigh in on this?

@bgaifullin
Copy link
Contributor Author

@kardianos I got your point, thank you for feedback.

@golang golang locked and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kardianos @bgaifullin @matipan @gopherbot