You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by "Dennis Patrone (JIRA)" <ji...@apache.org> on 2012/05/11 02:27:49 UTC
[jira] [Created] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Dennis Patrone created ACCUMULO-587:
---------------------------------------
Summary: 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Keith Turner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13277036#comment-13277036 ]
Keith Turner commented on ACCUMULO-587:
---------------------------------------
It would be interesting to verify this with a test. Derefernce the batch scanner and call System.gc() a few times and see if the log message is emitted. It would be nice to have the finalize message with a logs message if it works.
As for people relying on it, the finalize() method will be in impl not part of the public API. So you have to go digging around to find it. Could add some javadoc warning against relying on it.
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "John Vines (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13276928#comment-13276928 ]
John Vines commented on ACCUMULO-587:
-------------------------------------
finalize() gets called when garbage collection is about to be performed on the object. Unfortunately, I don't believe that garbage collection will ever occur on a non-closed Reader, because the threadpool existence would prevent garbage collection. This is the same deal with the BatchWriter. While putting in a finalize() would not actually cause harm in the code, I worry that it would lead users to believe they do not need to call close() to free up the resources involved.
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Billie Rinaldi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13278108#comment-13278108 ]
Billie Rinaldi commented on ACCUMULO-587:
-----------------------------------------
It appears to work. I ran this and saw the warning:
{noformat}
BatchScanner bs = conn.createBatchScanner("test", new Authorizations(), 1);
bs.setRanges(Collections.singleton(new Range()));
bs.iterator();
bs = null;
while (true)
System.gc();
{noformat}
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Aaron Cordova (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13278799#comment-13278799 ]
Aaron Cordova commented on ACCUMULO-587:
----------------------------------------
Keith - if the pool threads exit and users want to have a persistent BatchScanner for some reason, perhaps we can spin up the pool again when the user calls iterator() on it / makes use of it again.
> 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
[jira] [Resolved] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Billie Rinaldi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Billie Rinaldi resolved ACCUMULO-587.
-------------------------------------
Resolution: Fixed
Fix Version/s: 1.5.0
Looks good.
> 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
> Fix For: 1.5.0
>
>
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Billie Rinaldi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13278801#comment-13278801 ]
Billie Rinaldi commented on ACCUMULO-587:
-----------------------------------------
Dennis, I've committed the change to the batch reader. I'm going to add you to our contributors list in JIRA, which allows you more control over tickets. Any suggestions on how we can make finalize work for the batch writer?
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Dennis Patrone (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13278821#comment-13278821 ]
Dennis Patrone commented on ACCUMULO-587:
-----------------------------------------
Thanks! I'll look into the batch writer when I get a chance and let you know.
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Dennis Patrone (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13277003#comment-13277003 ]
Dennis Patrone commented on ACCUMULO-587:
-----------------------------------------
A minor change to the inner class BatchReaderThreadFactory (making it static and accepting the 'batchReaderInstance' in the constructor rather than relying on the instance member for thread naming) would allow a reader to be garbage collected without being closed-- even while the thread pool was alive.
I understand the concern that people might get lazy and rely on finalize() rather than call close() themselves. But I'd argue you can't help those with that kind of attitude. On the other hand, a finalize with a logged warning would help those that *inadvertently* forgot to call close and encounter a memory/thread leak to quickly recognize the source.
And if you are really concerned about lazy folks, you could choose to not even call close() in the finalize() method. That is, leave the leak but add the log warning. That way folks can't rely on the finalize() to free up resources involved, but the system warn those that forget to call close(); ultimately resulting in more stable Accumulo clients.
> 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Dennis Patrone (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Commented] (ACCUMULO-587) Add finalize to
TabletServerBatchReader to catch when user forgets to close
Posted by "Keith Turner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/ACCUMULO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13278167#comment-13278167 ]
Keith Turner commented on ACCUMULO-587:
---------------------------------------
Something else to look into may be making thread pool threads exit if idle for a certain amount of time.
> 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