fix: implement InstanceDisposer to close db connections #136
Conversation
| ds.Dispose() | ||
| count := 0 | ||
| conn.connections.Range(func(key, value interface{}) bool { | ||
| count++ | ||
| return true | ||
| }) | ||
| if count != 2 { | ||
| t.Errorf("missing connections") | ||
| if count != 0 { |
There was a problem hiding this comment.
Do we know why this was originally checking that we don't delete connections?
There was a problem hiding this comment.
I don't! Except that, of course, it originally didn't. I suppose it's possible that existing datasources are relying on that behavior, if their connectors reuse connections? But I feel like that would be a bit weird, and it doesn't look like any of ours do that.
You could argue that this should be a new major version, but to me this feels more like a bugfix than a backwards incompatible change, even if the bug was originally intentional. If that makes sense.
There was a problem hiding this comment.
(Fwiw I looked over the datasources - grafana and non-grafana - using sqlds.NewDatasource and it doesn't look like any of them reuse connections this way.)
There was a problem hiding this comment.
The change looks good to me. I'm a little surprised that we weren't actually closing the connections before 🤔 I would suggest releasing this as a major because it is a functionality change. I agree that it does seem like a bugfix also but we can't guarantee that any community plugins (if there are any) based on SQLds don't reuse connections in an unexpected fashion.
|
Fair enough - I looked over the github projects using it (only a handful outside of grafana) but github isn't the whole universe. I feel a little weird bumping to v5 just a couple months after v4, but so it goes :) |
This adds support for the
InstanceDisposer()interface toSQLDatasource, to fix a connection leak.Fixes #135