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

Get/Select SHOULD NOT release queries #25

Closed
jgiles opened this issue Nov 6, 2017 · 0 comments
Closed

Get/Select SHOULD NOT release queries #25

jgiles opened this issue Nov 6, 2017 · 0 comments

Comments

@jgiles
Copy link
Contributor

jgiles commented Nov 6, 2017

Currently, the Get and Select iterx methods call iter.ReleaseQuery(). This is a highly dangerous "convenience" which can lead to data races under common usage patterns.

The Iter constructor and Get and Select functions accept a query as a parameter, and so from an API design perspective it make most sense to let clients (who created the query) manage the query lifecycle.

E.g. it's a pretty common pattern to do

q := createQuery()
defer q.Release()
// do stuff with query, err returns, etc
return result, nil

But if "doing stuff" includes a gocqlx.Select, this pattern will lead to Release being called twice. That means the query object now has two entries in the query pool, which will distribute it to two separate goroutines, and racy concurrent read/writes ensue. We had a very hard-to-track-down bug in our service caused by this.

In this case, the cost of not calling Release is minimal; the query will just be GC'd instead of recycled. It's safer and better to let clients do releasing themselves.

jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Nov 6, 2017
Releasing query objects in Get/Select could easily lead to
double-releases, which can cause dangerous and tricky data races.
jgiles added a commit to paxosglobal/DEPRECATED-gocqlx that referenced this issue Nov 11, 2017
Releasing query objects in Get/Select could easily lead to
double-releases, which can cause dangerous and tricky data races.

Remove the query field of Iterx and its usages (all release-related).
This is a breaking API change, because it removes the exported method
ReleaseQuery.

Update documenting examples to demonstrate the deferred query release
pattern clients can use to manage query release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant