Solr
  1. Solr
  2. SOLR-930

SolrCore.close() : Warn in the logger when the internal reference count is > 0

    Details

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

      Java 6, Tomcat 6

      Description

      SolrCore.close() -> Add a warning statement when the internal reference count is > 0. ( as opposed to 0, as expected ) -

      1. SOLR-930.patch
        1 kB
        Karthik K
      2. SOLR-930.patch
        0.7 kB
        Karthik K

        Activity

        Hide
        Karthik K added a comment -

        — src/java/org/apache/solr/core/SolrCore.java (revision 727831)

        public void close() {
        int count = refCount.decrementAndGet();

        • if (count > 0) return;
          + if (count > 0) { + log.warn("Attempted close on " + this + " did not succeed because the new reference count " + count + " is > 0. "); + return; + }
        Show
        Karthik K added a comment - — src/java/org/apache/solr/core/SolrCore.java (revision 727831) public void close() { int count = refCount.decrementAndGet(); if (count > 0) return; + if (count > 0) { + log.warn("Attempted close on " + this + " did not succeed because the new reference count " + count + " is > 0. "); + return; + }
        Hide
        Ryan McKinley added a comment -

        added in trunk.
        thanks Kay

        Show
        Ryan McKinley added a comment - added in trunk. thanks Kay
        Hide
        Shalin Shekhar Mangar added a comment -

        Ryan, after this commit, I see the following message on almost every request to Solr. Just use the example jetty and request any JSP (stats/info) and you will find the warning in the log. Perhaps this logging is not needed?

        WARNING: Attempted close on org.apache.solr.core.SolrCore@b86944 did not succeed because the new reference count 1 is > 0
        Show
        Shalin Shekhar Mangar added a comment - Ryan, after this commit, I see the following message on almost every request to Solr. Just use the example jetty and request any JSP (stats/info) and you will find the warning in the log. Perhaps this logging is not needed? WARNING: Attempted close on org.apache.solr.core.SolrCore@b86944 did not succeed because the new reference count 1 is > 0
        Hide
        Karthik K added a comment -

        I believe the fact that refcount > 0 while closing is still a warning. If this were frequent enough - then probably some other resource manager, after incrementing the count is not decrementing the same after the use . We need to address that separately as opposed to commenting this one out.

        Show
        Karthik K added a comment - I believe the fact that refcount > 0 while closing is still a warning. If this were frequent enough - then probably some other resource manager, after incrementing the count is not decrementing the same after the use . We need to address that separately as opposed to commenting this one out.
        Hide
        Ryan McKinley added a comment -

        for now, i will just change the log level to "debug"

        Show
        Ryan McKinley added a comment - for now, i will just change the log level to "debug"
        Hide
        Yonik Seeley added a comment -

        The count being greater than zero is not an error condition - close() is called on every request.
        This patch should be reverted.

        Show
        Yonik Seeley added a comment - The count being greater than zero is not an error condition - close() is called on every request. This patch should be reverted.
        Hide
        Kay Kay added a comment -

        If it were not an error condition - something has to be abnormal since we are not releasing resources / calling closeHooks - upon close and return prematurely right at the beginning of the function.

        So - it indicates a possible resource leak that needs to be looked into, (possibly elsewhere ).

        I am not for reverting / changing the logging level .

        Show
        Kay Kay added a comment - If it were not an error condition - something has to be abnormal since we are not releasing resources / calling closeHooks - upon close and return prematurely right at the beginning of the function. So - it indicates a possible resource leak that needs to be looked into, (possibly elsewhere ). I am not for reverting / changing the logging level .
        Hide
        Karthik K added a comment -

        Reopening the issue - since close() is important to keep track of if the SolrCore is active ( if we were to re-register the core ,etc.)

        If count > 0 - close() returns silently as of now.

        If count < 0 , close() logs an error ( previously - it used to throwing an exception at least , to signify that we are into uncharted territory ).

        With the current behavior - it is impossible to actually know if close() indeed completed successfully with counter set to 0, and it will be hard to track memory references / reachability of object during memory leaks with the current behavior.

        Show
        Karthik K added a comment - Reopening the issue - since close() is important to keep track of if the SolrCore is active ( if we were to re-register the core ,etc.) If count > 0 - close() returns silently as of now. If count < 0 , close() logs an error ( previously - it used to throwing an exception at least , to signify that we are into uncharted territory ). With the current behavior - it is impossible to actually know if close() indeed completed successfully with counter set to 0, and it will be hard to track memory references / reachability of object during memory leaks with the current behavior.
        Hide
        Hoss Man added a comment -

        1) i think you are miss-understanding the point of close() ... it's called by every person who uses the core once they are done with it to indicate they are done with it. the resources are only truely freed once the count returns to 1 ... note the javadocs of hte method.

        2) if you want to know if all the resources have been freed (ie: everyone using the core has closed it) use "isClosed()"

        perhaps the confusion here is just the legacy name (close() was named back when SolrCore was a singleton i believe).

        Show
        Hoss Man added a comment - 1) i think you are miss-understanding the point of close() ... it's called by every person who uses the core once they are done with it to indicate they are done with it. the resources are only truely freed once the count returns to 1 ... note the javadocs of hte method. 2) if you want to know if all the resources have been freed (ie: everyone using the core has closed it) use "isClosed()" perhaps the confusion here is just the legacy name (close() was named back when SolrCore was a singleton i believe).
        Hide
        Karthik K added a comment -

        Thanks Hoss . That clarifies.

        Added @see to the javadoc to clarify more about it. Also - modified the javadoc about throwing exceptions if refCount < 0 (the code does not do it anymore ).

        The revised patch should be javadoc changes relevant to the part.

        Show
        Karthik K added a comment - Thanks Hoss . That clarifies. Added @see to the javadoc to clarify more about it. Also - modified the javadoc about throwing exceptions if refCount < 0 (the code does not do it anymore ). The revised patch should be javadoc changes relevant to the part.
        Hide
        Karthik K added a comment -

        Updating javadoc about not throwing exceptions .

        Added @see isClosed() for cross-reference.

        Show
        Karthik K added a comment - Updating javadoc about not throwing exceptions . Added @see isClosed() for cross-reference.
        Hide
        Hoss Man added a comment -

        Kay: @see tags can't contain

        {@link}

        tags (at least not in Javadoc 1.5)

        I took some of the text from your patch and reworked it into some new javadocs that are a bit more explicit about what exactly is going on under the hood...

        Committed revision 736518.

        Show
        Hoss Man added a comment - Kay: @see tags can't contain {@link} tags (at least not in Javadoc 1.5) I took some of the text from your patch and reworked it into some new javadocs that are a bit more explicit about what exactly is going on under the hood... Committed revision 736518.
        Hide
        Karthik K added a comment -
        Kay: @see tags can't contain {@link}

        tags (at least not in Javadoc 1.5)

        Oops - I was using java 6 (javadoc compiler). Thanks for the clarification in the docs.

        Show
        Karthik K added a comment - Kay: @see tags can't contain {@link} tags (at least not in Javadoc 1.5) Oops - I was using java 6 (javadoc compiler). Thanks for the clarification in the docs.

          People

          • Assignee:
            Ryan McKinley
            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