Solr
  1. Solr
  2. SOLR-914

Presence of finalize() in the codebase

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Component/s: clients - java
    • Labels:
      None
    • Environment:

      Tomcat 6, JRE 6

      Description

      There seems to be a number of classes - that implement finalize() method. Given that it is perfectly ok for a Java VM to not to call it - may be - there has to some other way

      { try .. finally - when they are created to destroy them }

      to destroy them and the presence of finalize() method , ( depending on implementation ) might not serve what we want and in some cases can end up delaying the gc process, depending on the algorithms.

      $ find . -name *.java | xargs grep finalize
      ./contrib/dataimporthandler/src/main/java/org/apache/solr/handler/dataimport/JdbcDataSource.java: protected void finalize() {
      ./src/java/org/apache/solr/update/SolrIndexWriter.java: protected void finalize() {
      ./src/java/org/apache/solr/core/CoreContainer.java: protected void finalize() {
      ./src/java/org/apache/solr/core/SolrCore.java: protected void finalize() {
      ./src/common/org/apache/solr/common/util/ConcurrentLRUCache.java: protected void finalize() throws Throwable {

      May be we need to revisit these occurences from a design perspective to see if they are necessary / if there is an alternate way of managing guaranteed destruction of resources.

      1. SOLR-914.patch
        4 kB
        Hoss Man
      2. SOLR-914.patch
        4 kB
        Noble Paul

        Issue Links

          Activity

          Hide
          Noble Paul added a comment -

          The fact that there are only 5 instances of these tells that it may not be an put inadvertently.

          two of them are my own creation. The code does not rely on these methods . But the consumers of these classes may forget to call the close/destroy methods explicitly . In such cases finalize() is just a fallback option

          Show
          Noble Paul added a comment - The fact that there are only 5 instances of these tells that it may not be an put inadvertently. two of them are my own creation. The code does not rely on these methods . But the consumers of these classes may forget to call the close/destroy methods explicitly . In such cases finalize() is just a fallback option
          Hide
          Karthik K added a comment -

          Precisely. I believe - we should get away with the same since in our case - when we try to shutdown and restart SolrCore - we are noticing instances where the gc thread postpones collecting the objects until finalize() is invoked.

          Given that finalize() is spec-d such that there is no guarantee of being called (and worse, if the implementation does decide , it does not collect the object until the method is invoked ).

          So - inserting a no-guarantee fallback option seems to doing more harm than good here.

          Show
          Karthik K added a comment - Precisely. I believe - we should get away with the same since in our case - when we try to shutdown and restart SolrCore - we are noticing instances where the gc thread postpones collecting the objects until finalize() is invoked. Given that finalize() is spec-d such that there is no guarantee of being called (and worse, if the implementation does decide , it does not collect the object until the method is invoked ). So - inserting a no-guarantee fallback option seems to doing more harm than good here.
          Hide
          Noble Paul added a comment -

          we are noticing instances where the gc thread postpones collecting the objects until finalize() is invoked.

          It is true but GC thread only has to wait for the time duration of the finalize() method call. So which class exactly is the culprit here in your case?

          Show
          Noble Paul added a comment - we are noticing instances where the gc thread postpones collecting the objects until finalize() is invoked. It is true but GC thread only has to wait for the time duration of the finalize() method call. So which class exactly is the culprit here in your case?
          Hide
          Karthik K added a comment -

          Primarily SolrCore and SolrIndexWriter ( specifically for my use-case ).

          Also - just noticed that - CoreContainer.finalize() ( calls shutdown() ) - has a synchronized block. While it is not a bottleneck for me , per se, (since I believe all through the life of the web-app , an instance of CoreContainer is alive and reachable , correct me if I am wrong here ). I believe we might need to revisit this if we were to extend this / provide orthogonal integration with other apps.

          Show
          Karthik K added a comment - Primarily SolrCore and SolrIndexWriter ( specifically for my use-case ). Also - just noticed that - CoreContainer.finalize() ( calls shutdown() ) - has a synchronized block. While it is not a bottleneck for me , per se, (since I believe all through the life of the web-app , an instance of CoreContainer is alive and reachable , correct me if I am wrong here ). I believe we might need to revisit this if we were to extend this / provide orthogonal integration with other apps.
          Hide
          Karthik K added a comment - - edited

          Separately - it might be worth to wrap around the code with a try .. finally

          { super.finalize(); }

          for all the custom finalizers for better code correctness.

          JIRA SOLR-924 logged for the same. Patch submitted for the new jira as well.

          Show
          Karthik K added a comment - - edited Separately - it might be worth to wrap around the code with a try .. finally { super.finalize(); } for all the custom finalizers for better code correctness. JIRA SOLR-924 logged for the same. Patch submitted for the new jira as well.
          Hide
          Karthik K added a comment -

          try {

          } finally

          { super.finalize(); }

          implemented for all existing finalizer code.

          Show
          Karthik K added a comment - try { } finally { super.finalize(); } implemented for all existing finalizer code.
          Hide
          Lance Norskog added a comment -

          A note: it is a good practice to use finalize() methods to check that a resource has already been released. It should log an error if the resource has not been released. Finalize() methods are all parked on one queue and that queue can back up. This can eventually cause the app to lock up. This is why it is not good to do I/O actions (like close a database connection) inside the finalize method.

          If the method only checks an internal marker, that will not cause a backup.

          Show
          Lance Norskog added a comment - A note: it is a good practice to use finalize() methods to check that a resource has already been released. It should log an error if the resource has not been released. Finalize() methods are all parked on one queue and that queue can back up. This can eventually cause the app to lock up. This is why it is not good to do I/O actions (like close a database connection) inside the finalize method. If the method only checks an internal marker, that will not cause a backup.
          Hide
          Karthik K added a comment -

          SolrIndexWriter#finalize() seems to delegate the same to IndexWriter.close() which is quite expensive.

          ConcurrentLRUCache#finalize() seems to close a thread (by means of notify() ) . I am not sure if those methods are good enough candidates to be present in finalize() since they seem to do more than logging at this point.

          Show
          Karthik K added a comment - SolrIndexWriter#finalize() seems to delegate the same to IndexWriter.close() which is quite expensive. ConcurrentLRUCache#finalize() seems to close a thread (by means of notify() ) . I am not sure if those methods are good enough candidates to be present in finalize() since they seem to do more than logging at this point.
          Hide
          Hoss Man added a comment -

          IndexWriter.close() is very cheap if the IndexWriter is already closed ... if it's not already closed, then doing so in the finalize() method is our last resort. (but I would think a patch to IndexWriter to make it explicitly implement an "isClosed() method would certainly be nice to help keep the code clean and make it possible to log a warning).

          Ditto for ConcurrentLRUCache ... finalize calls destroy which sets the stop flag and notifies the thread ... calling destroy() again shouldn't be that expensive if the client has already called it (which FastLRUCache does) – but changing the code to record when destroy() has been called, and log a warning in finalize if it hasn't been called yet (then calling it) seems like a good idea.

          Show
          Hoss Man added a comment - IndexWriter.close() is very cheap if the IndexWriter is already closed ... if it's not already closed, then doing so in the finalize() method is our last resort. (but I would think a patch to IndexWriter to make it explicitly implement an "isClosed() method would certainly be nice to help keep the code clean and make it possible to log a warning). Ditto for ConcurrentLRUCache ... finalize calls destroy which sets the stop flag and notifies the thread ... calling destroy() again shouldn't be that expensive if the client has already called it (which FastLRUCache does) – but changing the code to record when destroy() has been called, and log a warning in finalize if it hasn't been called yet (then calling it) seems like a good idea.
          Hide
          Mark Miller added a comment -

          I agree with Kay Kay and Lance here - I don't think we should be doing any closing/shutdown with finalize. Logging a warning seems like the right way to go to me. This is the type of thing that hides problems, and it just doesn't do it cleanly.

          Show
          Mark Miller added a comment - I agree with Kay Kay and Lance here - I don't think we should be doing any closing/shutdown with finalize. Logging a warning seems like the right way to go to me. This is the type of thing that hides problems, and it just doesn't do it cleanly.
          Hide
          Noble Paul added a comment -

          what do we plan to do with this?

          Show
          Noble Paul added a comment - what do we plan to do with this?
          Hide
          Noble Paul added a comment -

          Logging a warning seems like the right way to go to me

          so do you mean , logging a warning and do not do cleanup. or log a warning and do the cleanup ?

          I would prefer the latter because the system will continue to work

          Show
          Noble Paul added a comment - Logging a warning seems like the right way to go to me so do you mean , logging a warning and do not do cleanup. or log a warning and do the cleanup ? I would prefer the latter because the system will continue to work
          Hide
          Noble Paul added a comment -

          all the finalize () methods check once if the object is already closed. if not log a warning and close

          Show
          Noble Paul added a comment - all the finalize () methods check once if the object is already closed. if not log a warning and close
          Hide
          Karthik K added a comment -

          What I meant is (and others who had commented on the jira seem to concur) - logging a warning is ok.

          Code to release resources should be avoided as a finalize is no equivalent to a C++ dtor.

          This patch does not seem to address the issue.

          Show
          Karthik K added a comment - What I meant is (and others who had commented on the jira seem to concur) - logging a warning is ok. Code to release resources should be avoided as a finalize is no equivalent to a C++ dtor. This patch does not seem to address the issue.
          Hide
          Noble Paul added a comment -

          Code to release resources should be avoided as a finalize is no equivalent to a C++ dtor.

          yes. But if the user has forgotten to do so It is not a good idea to punish him by blowing up. A warning should be enough.

          Show
          Noble Paul added a comment - Code to release resources should be avoided as a finalize is no equivalent to a C++ dtor. yes. But if the user has forgotten to do so It is not a good idea to punish him by blowing up. A warning should be enough.
          Hide
          Noble Paul added a comment -

          There seems to be no consensus on the fix. Unassigning myself

          Show
          Noble Paul added a comment - There seems to be no consensus on the fix. Unassigning myself
          Hide
          Hoss Man added a comment -

          I don't understand objections to the idea that finalize should close if (and only it) the resource hasn't already been closed ... people shouldn't relying on it, but having code that aids in the prevention of resource leaks doesn't seem like abad thing to me.

          the only things i would change about this patch...

          • make the logging done by the finalizer methods more serious (error or maybe even fatal) and make them convey why it's an error ("...was not closed prior to finalizing")
          • SolrIndexWriter.finalize() still calls super.close() (only this.close() should ever call super.close())
          Show
          Hoss Man added a comment - I don't understand objections to the idea that finalize should close if (and only it) the resource hasn't already been closed ... people shouldn't relying on it, but having code that aids in the prevention of resource leaks doesn't seem like abad thing to me. the only things i would change about this patch... make the logging done by the finalizer methods more serious (error or maybe even fatal) and make them convey why it's an error ("...was not closed prior to finalizing") SolrIndexWriter.finalize() still calls super.close() (only this.close() should ever call super.close())
          Hide
          Lance Norskog added a comment -

          Yes, Solr should keep working. But somewhere in the logs (that a few people read) it should note that the resource should have been closed before the finalize. So, yes, the patch is right in that it does the shutdown codes inside try{} blocks that ignore errors.

          +1 on on the current patch and the policy it implements.

          Show
          Lance Norskog added a comment - Yes, Solr should keep working. But somewhere in the logs (that a few people read) it should note that the resource should have been closed before the finalize. So, yes, the patch is right in that it does the shutdown codes inside try{} blocks that ignore errors. +1 on on the current patch and the policy it implements.
          Hide
          Hoss Man added a comment -

          revised patch with the improvements i mentioned earlier, also fixes a cut/paste mistake in one of the log messages.

          Show
          Hoss Man added a comment - revised patch with the improvements i mentioned earlier, also fixes a cut/paste mistake in one of the log messages.
          Hide
          Noble Paul added a comment -

          I guess this should be enough.

          Show
          Noble Paul added a comment - I guess this should be enough.
          Hide
          Hoss Man added a comment -

          Committed revision 807872.

          FYI: this patch surfaced some bugs in TestFastLRUCache where the cache wasn't being closed properly.

          Show
          Hoss Man added a comment - Committed revision 807872. FYI: this patch surfaced some bugs in TestFastLRUCache where the cache wasn't being closed properly.
          Hide
          Grant Ingersoll added a comment -

          Bulk close for Solr 1.4

          Show
          Grant Ingersoll added a comment - Bulk close for Solr 1.4

            People

            • Assignee:
              Hoss Man
              Reporter:
              Karthik K
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 480h
                480h
                Remaining:
                Remaining Estimate - 480h
                480h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development