always close db connection on error in Open

[charlievieth]

This commit fixes a bug where the db connection would not always be closed when an error occurred in Open. Additionally, it makes sure that we always unregister any callbacks associated with the connection, which previously did not always happen. This change consolidates the error handling logic (previously, it had to be done for each return statement) which should make it more difficult to introduce this type of bug in the future.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 14.28571% with 90 lines in your changes missing coverage. Please review.

Project coverage is 47.60%. Comparing base (18cdded) to head (0ae71f8).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
sqlite3.go	14.28%	69 Missing and 21 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   47.16%   47.60%   +0.44%     
==========================================
  Files          12       12              
  Lines        1533     1523      -10     
==========================================
+ Hits          723      725       +2     
+ Misses        669      657      -12     
  Partials      141      141              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mattn]

Please fix conflict

[charlievieth]

Conflict fixed and thank you for the quick review!

[rittneje]

Instead of this, we can use a named return value and a defer. That will also enable closing it on panic, which is another hole we have today.

defer func() {
    if retErr != nil {
        conn.Close()
    }
}()

[charlievieth]

That would handle closing the conn on error, but if there is a panic the named return error value will almost certainly be nil since the panic will occur before the return statement is executed. We could use a deferred call to recover if we really cared about closing the conn and releasing its resources:

	defer func() {
		if e := recover(); e != nil {
			conn.Close()
			panic(e)
		}
	}()

IMHO, any panic here should probably be considered terminal so I don't think we need to care too much about leaking resources, but also happy to make the change since this is an externally managed resource.

[rittneje]

Sorry, I was a little unclear. I just meant that using a defer would enable accounting for panics, not the named return itself.

The reason to close in the panic case is that this library cannot control what the caller of Open does. In particular, they could choose to swallow panics, and it would be good to at least avoid causing a memory leak in that situation.

[charlievieth]

Sorry, I was a little unclear. I just meant that using a defer would enable accounting for panics, not the named return itself.

Thanks and yeah was a little confused since in the given code snippet the defer would not close the db conn in the event of a panic (since the named return value would likely be nil).

I updated the code to close the db connection if a panic occurs during setup.