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 be able to return an error #83

Closed
wdullaer opened this issue Mar 8, 2024 · 3 comments
Closed

sqlitex.Pool.Get should be able to return an error #83

wdullaer opened this issue Mar 8, 2024 · 3 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@wdullaer
Copy link

wdullaer commented Mar 8, 2024

Obtaining a connection from the Pool is an operation that can fail.
From the description:

If no connection is available, Get will block until at least one connection is returned with Pool.Put, or until either the Pool is closed or the context is canceled. If no connection can be obtained, nil is returned.

It would be nice that if an explicit error can be returned, especially because the two modes that lead to a nil connection are very different:

  • If the Pool is closed: this is in most scenario's a coding bug and can be ruled out by path analysis
  • If the context is canceled: this is part of normal runtime behaviour and needs to be handled

If the error returned by Get would wrap the context error, we could figure out in client code what has happened and deal with it appropriately.

I became aware of this, because I didn't fully realize that a canceled context would return a nil connection, which lead to panics of the program in production.

I realize this is a breaking change from an API perspective, but it would express much better what's going on and hopefully prevent other people from learning the hard way.

Bonus points: it'd also be nice if the error that's returned by Exec when you do a query on a connection associated with an expired context, would wrap the context error.

@zombiezen zombiezen added enhancement New feature or request bug Something isn't working labels Mar 17, 2024
@zombiezen
Copy link
Owner

Agreed, especially with the preparation logic introduced in #65. I'm juggling a few other priorities at the moment, but will try to introduce this soon. I think we can still introduce this as a non-breaking change and just deprecate sqlitex.Pool.Get.

As an aside, nil connections are supposed to just cause errors instead of panics, because this sort of thing has come up before. Do you have specific places inside the library that panic on a nil connection?

@wdullaer
Copy link
Author

Good to know.
We're not in a rush, we wrote a small wrapper around Get which tries to do the correct error handling.

I would need to take a look at the crash data again to confirm, but I think the panic happened inside of sqlitex.Execute.

@zombiezen
Copy link
Owner

Added a new method Take to handle this, which will be available in the next release.

I added a couple tests to check sqlitex.Execute and sqlitex.ExecuteTransient to verify they act correctly in the face of a (*sqlite.Conn)(nil). I was not able to uncover anything, so if you encounter this down the road, please open a new issue. (Although I'd imagine this failure mode will happen less now, since Pool.Get has been the footgun that introduces these nils for a long time.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants