Fix two sources of SQL statement leaks. #13259
Conversation
1) SqlTaskResource and DruidJdbcResultSet leaked statements 100% of the time, since they call stmt.plan(), which adds statements to SqlLifecycleManager, and they do not explicitly remove them. 2) SqlResource leaked statements if yielder.close() threw an exception. (And also would not emit metrics, since in that case it failed to call stmt.close as well.)
|
I tested this patch by running |
| @@ -301,6 +302,7 @@ public void cancel() | |||
| public void close() | |||
| { | |||
| if (state != State.START && state != State.CLOSED) { | |||
| // super.close calls closeQuietly, which removes us from the sqlLifecycleManager. | |||
There was a problem hiding this comment.
Nit: should we rely on close/closeQuietly to do the right thing?
Since we are anyway doing an explicit remove in closeWithError, I suppose we could do the same here too. Then we can just not override closeQuietly.
Alternatively,
I think closeWithQuietly, close, closeWithError should all internally call a cleanup method. This new method should be abstract in AbstractStatement so that all impls provide a concrete cleanup method.
There was a problem hiding this comment.
Ah, now that I read this again, I see that closeQuietly is called by both close and closeWithError. So we can do the second thing you suggested: only clean up the statement there. I just pushed up another commit with this change.
SqlTaskResource and DruidJdbcResultSet leaked statements 100% of
the time, since they call stmt.plan(), which adds statements to
SqlLifecycleManager, and they do not explicitly remove them.
SqlResource leaked statements if yielder.close() threw an exception.
(And also would not emit metrics, since in that case it failed to
call stmt.close as well.)
Item (2) has been around for a while. Item (1) is a regression in 24.0, introduced in #12845. That patch moved SqlLifecycleManager registration from SqlResource to DirectStatement, but kept de-registration in SqlResource only.