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

sqlitex.Pool.Get should never return a nil Conn — click here to learn more! #17

Closed
peterbourgon opened this issue Aug 10, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@peterbourgon
Copy link

Right now, sqlitex.Pool.Get can return a nil Conn in a few circumstances, for example if the pool has been closed, or if the provided context has been canceled. This means consumers can't safely use the normal Get/defer Put idiom.

conn := pool.Get(ctx)
defer pool.Put(conn)
// use conn

And instead must do an explicit nil check within each stanza.

conn := pool.Get(ctx)
if conn == nil {
    // return
}
defer pool.Put(conn)
// use conn

It would be better if the Get method always returned a non-nil Conn, and, in the situations where it's not actually usable, have all of its methods short-circuit and return an appropriate error.

@zombiezen
Copy link
Owner

zombiezen commented Aug 10, 2021 via email

@peterbourgon
Copy link
Author

I'd be open to having a nil *sqlite.Conn return errors. Would that address your needs?

Ah, you mean if all the methods did a nil check on the receiver? Yeah, Totes McGotes 👍 And if pool.Put &c. handled a nil Conn, too.

@peterbourgon
Copy link
Author

peterbourgon commented Aug 15, 2021

@zombiezen Does that cover sqlitex.Exec-class functions?

edit: I reckon it does, just want the explicit confirmation :)

@zombiezen
Copy link
Owner

It should, since all of the sqlitex.Exec methods are implemented in terms of methods on *sqlite.Conn. I haven't tested too extensively, so if it doesn't cover it, feel free to reopen.

@peterbourgon
Copy link
Author

conn := pool.Get(ctx)
defer pool.Put(conn)

This still panics if conn is nil.

@peterbourgon
Copy link
Author

I can't seem to re-open? lol github

@zombiezen ← mentioning just in case

@zombiezen zombiezen reopened this Aug 17, 2021
@zombiezen
Copy link
Owner

Oh right, Pool.Put needed to be changed. 🤦 I'll have a fix today.

@zombiezen zombiezen added the enhancement New feature or request label Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants