accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dennis Patrone (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-587) Add finalize to TabletServerBatchReader to catch when user forgets to close
Date Tue, 22 May 2012 23:18:41 GMT

    [ 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

        

Mime
View raw message