[
https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13281299#comment-13281299
]
Dennis Patrone commented on ACCUMULO-587:
-----------------------------------------
I finally got a chance to look into this. I don't think anything special needs
to be done for {{MultiTableBatchWriterImpl}} other than adding the
{{finalize()}}. The object that actually needs to be closed (the
{{TabletServerBatchWriter}}) is private and not shared. The {{BatchWriter}}
implementation returned by the {{getBatchWriter(String table)}} method is a
non-static inner class and therefore all references to any {{BatchWriter}}
created by the {{MultiTableBatchWriterImpl}} must be lost before {{finalize()}}
could be called.
The only wrinkle with {{MultiTableBatchWriterImpl}} is the {{close()}} method
may throw a {{MutationsRejectedException}}, so that must be handled in the
{{finalize()}} method. I'm not sure if you'd want to turn that into a logged
'non-exception' or a {{RuntimeException}}; I could see the argument for either
way (although I'd lean toward the {{RuntimeException}}).
Regardless of the choice for handling the {{MutationsRejectedException}}, the
memory/thread leak would be avoided because {{TabletServerBatchWriter}}'s
{{close()}} method closes the thread pool in a finally block even if it throws
the {{MutationsRejectedException}}.
So I think just adding a method like this to {{MultiTableBatchWriterImpl}}
provides the needed check:
{code}
@Override
protected void finalize() {
if (!closed) {
log.warn(MultiTableBatchWriterImpl.class.getSimpleName()
+ " not shutdown; did you forget to call close()?");
try {
close();
} catch (MutationsRejectedException mre) {
log.error(MultiTableBatchWriterImpl.class.getSimpleName()
+ " internal error.", mre);
// ... AND/OR ...
throw new RuntimeException("Exception when closing "
+ MultiTableBatchWriterImpl.class.getSimpleName(), mre);
}
}
}
{code}
> Add finalize to TabletServerBatchReader to catch when user forgets to close
> ---------------------------------------------------------------------------
>
> Key: ACCUMULO-587
> URL: https://issues.apache.org/jira/browse/ACCUMULO-587
> Project: Accumulo
> Issue Type: Improvement
> Components: client
> Reporter: Dennis Patrone
> Assignee: Billie Rinaldi
> Priority: Trivial
>
> If a client forgets to close a BatchScanner or BatchDeleter, threads are
> leaked in the TabletServerBatchReader implementation. It would be nice if a
> finalize method were added to check and warn the user of such a problem. The
> thread pool appeared to only be shared with the
> TabletServerBatchReaderIterator, which maintains a reference to the
> TabletServerBatchReader itself. So AFAICT if the TabletServerBatchReader is
> eligible for garbage collection, there can be no client references to that
> scanner or any iterators it created (i.e., it _should_ have been closed).
> For example:
> {code}
> protected void finalize() {
> if (!queryThreadPool.isShutdown()) {
> // add a logger reference in class initialization
> log.warn("TabletServerBatchReader not shutdown; did you forget to call
> close()?");
> close();
> }
> }
> {code}
> The same might be true for the TabletServerBatchWriter (it has a close), but
> I didn't look into that class.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
