Fix connection leak on conn.ssl or conn.startup failure #698
Conversation
| cn.c.Close() | ||
| } | ||
| }() | ||
|
|
||
| cn.ssl(o) |
There was a problem hiding this comment.
It would be ideal if these functions returned errors instead of panicking... but this works as a hot fix. Would leave that to maintainers.
https://github.com/golang/go/wiki/CodeReviewComments#dont-panic
There was a problem hiding this comment.
Indeed. Ideally cn.startup would return an error. But that's a larger change, so I'm fine with this now.
| // cn.ssl and cn.startup panic on error. Make sure we don't leak cn.c. | ||
| panicking := true | ||
| defer func() { | ||
| if panicking { |
There was a problem hiding this comment.
Since err is a named return value I think you can just do if err != nil and drop panicking.
There was a problem hiding this comment.
I thought so, too, initially, but you can't. This defer runs before the recover-defer, so err will still be nil here.
There was a problem hiding this comment.
Ugh. I'm not aware of a project that is more popular and more un-Goish than lib/pq.
| cn.c.Close() | ||
| } | ||
| }() | ||
|
|
||
| cn.ssl(o) |
There was a problem hiding this comment.
Indeed. Ideally cn.startup would return an error. But that's a larger change, so I'm fine with this now.
|
The build errors aren't your fault: due to the golang tools repo using stuff not present in older Go versions. We should fix that, or more probably just stop testing unsupported versions of Go. |
Fixes #697