Solr
  1. Solr
  2. SOLR-924

Solr: Making finalizers call super.finalize() wrapped in try..finally block

    Details

      Description

      There are some occurences of finalizers in the code base. While the presence of them is debatable and discussed in a separate JIRA - the ones that are retained are better off wrapped around a try .. finally block to recursively call the finalizer of the super class for proper resource usage unwinding , (in case finalizers get invoked ).

        Issue Links

          Activity

          Hide
          Karthik K added a comment -

          The following classes have their try .. finally clause implemented in finalizers set up for recursive call.

          Index: contrib/dataimporthandler/src/main/java/org/apache/solr/handler/dataimport/JdbcDataSource.java
          Index: src/java/org/apache/solr/update/SolrIndexWriter.java
          Index: src/java/org/apache/solr/core/SolrCore.java
          Index: src/java/org/apache/solr/core/CoreContainer.java
          Index: src/common/org/apache/solr/common/util/ConcurrentLRUCache.java

          Show
          Karthik K added a comment - The following classes have their try .. finally clause implemented in finalizers set up for recursive call. Index: contrib/dataimporthandler/src/main/java/org/apache/solr/handler/dataimport/JdbcDataSource.java Index: src/java/org/apache/solr/update/SolrIndexWriter.java Index: src/java/org/apache/solr/core/SolrCore.java Index: src/java/org/apache/solr/core/CoreContainer.java Index: src/common/org/apache/solr/common/util/ConcurrentLRUCache.java
          Hide
          Noble Paul added a comment -

          Why do we need to call a super.finalize() when the super class is java.lang.Object ?

          It achieves no purpose

          Show
          Noble Paul added a comment - Why do we need to call a super.finalize() when the super class is java.lang.Object ? It achieves no purpose
          Hide
          Karthik K added a comment -

          Not all of them seems to inherit from Object ( JdbcDataSource inherits from DataSource<?> ). SolrIndexWriter from IndexWriter etc. So - to ensure proper cleanup of resources - we need to be having the try .. finally block around finalizers.

          Show
          Karthik K added a comment - Not all of them seems to inherit from Object ( JdbcDataSource inherits from DataSource<?> ). SolrIndexWriter from IndexWriter etc. So - to ensure proper cleanup of resources - we need to be having the try .. finally block around finalizers.
          Hide
          Noble Paul added a comment -

          DataSource<?> is just an interface defined as an abstract class. For others it may be worth it.

          We do not have to be religious about this. if the superclass is doing something in the finalize then it is worth adding this or else we can drop it

          Show
          Noble Paul added a comment - DataSource<?> is just an interface defined as an abstract class. For others it may be worth it. We do not have to be religious about this. if the superclass is doing something in the finalize then it is worth adding this or else we can drop it
          Hide
          Karthik K added a comment -
          DataSource<?> is just an interface defined as an abstract class. For others it may be worth it.

          I believe we might as well be consistent when making changes instead of excluding based on specific super class implementation.

          Show
          Karthik K added a comment - DataSource<?> is just an interface defined as an abstract class. For others it may be worth it. I believe we might as well be consistent when making changes instead of excluding based on specific super class implementation.
          Hide
          Hoss Man added a comment -

          What Kay is suggesting is a pretty well established "best practice" .. even if you know the super class has an empty finalize method it's still a good idea to call super.finalize() in a finally block because you have no garuntee someone else won't modify the implementation of the super class in a future version. even when you subclass Object, super.finalize() is a good idea to protect yourself against yourself: future refactoring could inject a new super class (which could then gain a non-trivial finalize method)

          i really see no downside to the patch

          Show
          Hoss Man added a comment - What Kay is suggesting is a pretty well established "best practice" .. even if you know the super class has an empty finalize method it's still a good idea to call super.finalize() in a finally block because you have no garuntee someone else won't modify the implementation of the super class in a future version. even when you subclass Object, super.finalize() is a good idea to protect yourself against yourself: future refactoring could inject a new super class (which could then gain a non-trivial finalize method) i really see no downside to the patch
          Hide
          Hoss Man added a comment -

          Committed revision 728336.

          thanks Kay.

          Show
          Hoss Man added a comment - Committed revision 728336. thanks Kay.
          Hide
          Karthik K added a comment -

          Thanks HossMan for committing the same.

          Show
          Karthik K added a comment - Thanks HossMan for committing the same.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

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

                  Development