GH-932: [JDBC] Fix memory leak on Connection#close due to unclosed Statement(s) - #933
Conversation
Sorry, something went wrong.
This comment has been minimized.
|
I'm not sure if I should further refactor the logic on ArrowFlightConnection.close() The changes so far are mostly to address the bug at #932 so I didn't touch the old logic a lot. However, the way things are is that if an Exception gets thrown for example in AutoCloseables.close(clientHandler); we immediately throw the Exception and don't attempt to clean the rest of the resources. I wonder if it would make more sense to use a logic similar to AutoCloseables.close() for every resource, and proceed with the cleanup until the end, and then throwing all the Exceptions generated. The class ArrowFlightJdbcVectorSchemaRootResultSet uses this approach I'm mentioning for its close method. Let me know if you want me to do that refactoring in this or another PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, I think it would be great to just build up a single list of AutoCloseables that we can pass to AutoCloseables so that an exception in one doesn't skip the rest, and hopefully that will clean up the logic too.
Sorry, something went wrong.
There was a problem hiding this comment.
I can't build a single list of AutoCloseables because ExecutorService isn't AutoCloseable before Java 19. I built a list with the ones I could, and left the try catch logic for the rest. Is this ok?
A question that hit me while doing this was what MT-safety guarantees we have on the implementation of the JDBC driver. The JDBC 4.3 spec doesn't speak about whether implementations of interfaces such as Connection need to be MT-safe, but older versions of the spec do speak about it: https://web.archive.org/web/20090213013250/http://java.sun.com/j2se/1.3/docs/guide/jdbc/spec/jdbc-spec.frame9.html
It seems to me like we are assuming each Connection is only used in a single thread, is this documented somewhere?
Sorry, something went wrong.
jbonofre
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
What's Changed
Closing a Connection when there was one or more ResultSet that matched the following 2 conditions
would generate exceptions due to memory leaks.
Now, closing a Connection will first close all the Statement instances obtained via that Connection,
which has a side effect of closing all the ResultSet, and then proceed with the old closing logic. This
side effect is guaranteed by the JDBC Spec 4.3, chapter 13.1.4
The old closing logic was also slightly refactored to:
Closes #932.