chore: close db properly in early exit paths in ConnectToPostgres #18448
Conversation
471bd6d to
e9f833b
Compare
cli/server.go
Outdated
| err = xerrors.Errorf("no rows returned for version select: %w", version.Err()) | ||
| return nil, err |
There was a problem hiding this comment.
Can we just name this error something else? Like dbError?
I say that because a few lines up is version, err := sqlDB.QueryContext(ctx, "SHOW server_version_num;"). Does that not shadow the err variable and break this assignment to the correct err here?
There was a problem hiding this comment.
Based on the language spec, this statement assigns to the correct err:
Unlike regular variable declarations, a short variable declaration may redeclare variables provided they were originally declared earlier in the same block (or the parameter lists if the block is the function body) [...] Redeclaration does not introduce a new variable; it just assigns a new value to the original.
Either way, it's non-obvious and if somebody modified this function in the future, it's likely they would forget about the err assignment/close db logic. I added a dbNeedsClosing variable and refactored the defer statement to use it instead of err. This way, the db will always be closed unless a code path explicitly opts out of it.
There was a problem hiding this comment.
TIL! Sorry I was misinformed, I had no idea that was the effect.
e9f833b to
4aacca3
Compare
4aacca3 to
6d07d78
Compare
There were some code paths where if we exited early from the function the postgres connection would never get cleaned up.
This is the mechanism that cleans up the db - it requires the err variable to be not nil:
coder/cli/server.go
Lines 2319 to 2328 in 118bf98