-
Notifications
You must be signed in to change notification settings - Fork 80
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
vai: do not discard closing statement errors #462
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Silvio Moioli <[email protected]>
pkg/sqlcache/informer/indexer.go
Outdated
defer func() { | ||
cerr := i.CloseStmt(stmt) | ||
if cerr != nil { | ||
err = errors.Join(err, cerr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the error message from cerr
provide enough context to know that it came from this close statement call (or just allow us to troubleshoot)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, but that really depends on the SQLite library implementation.
I could add the statement as context if you prefer that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's not too much trouble yes that'd be great to have a bit more context, just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed as 7d290ce
TIL: the current library actually never returns errors on close - but I still feel like this code is good to have, as it might change and/or we might switch to another library which might
current https://gitlab.com/cznic/sqlite/-/blob/v1.34.5/sqlite.go#L472-479
possible https://github.com/mattn/go-sqlite3/blob/7658c06970ecf5588d8cd930ed1f2de7223f1010/sqlite3.go#L1896-L1915
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Straightforward
Signed-off-by: Silvio Moioli <[email protected]>
Errors in deferred
CloseStmt
calls are currently ignored.This patch captures such errors and returns them (possibly joined with another error if it happened).
Part of rancher/rancher#47825