Solr
  1. Solr
  2. SOLR-8451

We should not call method.abort in HttpSolrClient and HttpSolrCall#remoteQuery should not close streams.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None
    1. SOLR-8451.patch
      18 kB
      Mark Miller
    2. SOLR-8451.patch
      28 kB
      Mark Miller
    3. SOLR-8451.patch
      18 kB
      Mark Miller
    4. SOLR-8451.patch
      19 kB
      Mark Miller
    5. SOLR-8451.patch
      7 kB
      Mark Miller

      Issue Links

        Activity

        Show
        Mark Miller added a comment - See https://issues.apache.org/jira/browse/SOLR-7339?focusedCommentId=15066701&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15066701 and https://issues.apache.org/jira/browse/SOLR-7339?focusedCommentId=15066716&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15066716
        Hide
        Mark Miller added a comment -

        "I moved the method abort call out of 'shouldClose' - it doesn't seem like it should be tied up in that.
        I also made it first. I was just kind of guessing on that, but it looks like it should be first according to: https://hc.apache.org/httpcomponents-client-ga/httpclient/examples/org/apache/http/examples/client/ClientAbortMethod.java"

        Show
        Mark Miller added a comment - "I moved the method abort call out of 'shouldClose' - it doesn't seem like it should be tied up in that. I also made it first. I was just kind of guessing on that, but it looks like it should be first according to: https://hc.apache.org/httpcomponents-client-ga/httpclient/examples/org/apache/http/examples/client/ClientAbortMethod.java "
        Hide
        Mark Miller added a comment -
        Index: solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java
        ===================================================================
        --- solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java	(revision 1720969)
        +++ solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java	(working copy)
        @@ -589,14 +589,16 @@
               throw new SolrServerException(
                   "IOException occured when talking to server at: " + getBaseURL(), e);
             } finally {
        -      if (respBody != null && shouldClose) {
        -        try {
        -          respBody.close();
        -        } catch (IOException e) {
        -          log.error("", e);
        -        } finally {
        -          if (!success) {
        -            method.abort();
        +      try {
        +        if (!success) {
        +          method.abort();
        +        }
        +      } finally {
        +        if (respBody != null && shouldClose) {
        +          try {
        +            respBody.close();
        +          } catch (IOException e) {
        +            log.error("", e);
                   }
                 }
               }
        
        Show
        Mark Miller added a comment - Index: solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java =================================================================== --- solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java (revision 1720969) +++ solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java (working copy) @@ -589,14 +589,16 @@ throw new SolrServerException( "IOException occured when talking to server at: " + getBaseURL(), e); } finally { - if (respBody != null && shouldClose) { - try { - respBody.close(); - } catch (IOException e) { - log.error("", e); - } finally { - if (!success) { - method.abort(); + try { + if (!success) { + method.abort(); + } + } finally { + if (respBody != null && shouldClose) { + try { + respBody.close(); + } catch (IOException e) { + log.error("", e); } } }
        Hide
        Mark Miller added a comment -

        I'm evolving this patch in SOLR-8453. Quote from that issue:

        I think we overdo method.abort and I think it messes up connection reuse. On a clean server->client exception, we should simply make sure the response entity content is fully consumed and closed like a normal request.

        Show
        Mark Miller added a comment - I'm evolving this patch in SOLR-8453 . Quote from that issue: I think we overdo method.abort and I think it messes up connection reuse. On a clean server->client exception, we should simply make sure the response entity content is fully consumed and closed like a normal request.
        Hide
        Mark Miller added a comment -

        Okay, I dug into this a bit. Actually, we should not call abort at all. Right now, since we call it after close, it's harmless though.

        Also, ConcurrentUpdateSolrClient very oddly seem to always not read that last 40 or 44 bytes of the server response even on successful requests. That's odd, but should not mess with connection reuse. We only have to ensure we close the content stream, not read it all.

        Show
        Mark Miller added a comment - Okay, I dug into this a bit. Actually, we should not call abort at all. Right now, since we call it after close, it's harmless though. Also, ConcurrentUpdateSolrClient very oddly seem to always not read that last 40 or 44 bytes of the server response even on successful requests. That's odd, but should not mess with connection reuse. We only have to ensure we close the content stream, not read it all.
        Hide
        Karl Wright added a comment -

        I don't follow the reasoning here. AFAICT method.abort should be called whenever there's any chance that the response has not been fully read, which would be the case if an exception was thrown.

        Show
        Karl Wright added a comment - I don't follow the reasoning here. AFAICT method.abort should be called whenever there's any chance that the response has not been fully read, which would be the case if an exception was thrown.
        Hide
        Mark Miller added a comment -

        whenever there's any chance that the response has not been fully read

        No documentation I can find says that.

        If you make a little test, you will see that method.abort prevents connection reuse. When Solr throws expected errors for updates, we don't want to poison the connection, we want it to go back to the pool. If we actually had to read the full stream, it would be better to just do that. But again, testing and any documentation I can find shows that we just have to close the stream - we don't have to read it all. And connections are released happily back to the pool for reuse.

        There is no need for abort here. Nor was it even having an affect when it was called after the content stream close (which it always was).

        Show
        Mark Miller added a comment - whenever there's any chance that the response has not been fully read No documentation I can find says that. If you make a little test, you will see that method.abort prevents connection reuse. When Solr throws expected errors for updates, we don't want to poison the connection, we want it to go back to the pool. If we actually had to read the full stream, it would be better to just do that. But again, testing and any documentation I can find shows that we just have to close the stream - we don't have to read it all. And connections are released happily back to the pool for reuse. There is no need for abort here. Nor was it even having an affect when it was called after the content stream close (which it always was).
        Hide
        Karl Wright added a comment -
        Show
        Karl Wright added a comment - Hmm, under some circumstances this is definitely not the right thing to do. http://stackoverflow.com/questions/1565349/cancel-abort-a-connection-from-the-threadsafeclientconnmanager-connection-pool
        Hide
        Mark Miller added a comment -

        It's the right thing to do. I have read the doc, the code, and mailing list info. I have done tests.

        After you are done using a connection you need to do either two things: abort it or close the content stream. If you abort it, you won't reuse the connection. There is no need to read through the whole inputstream. There is no need to use abort in normal safe code unless you need that as a feature to interrupt a request. All the proper example code shows the pattern to use is to just close the content input stream.

        In the end, it's really simple to test by hand and see what causes connection reuse and what breaks it.

        It was a mistake to add the abort call, but luckily it never had an effect - closing the content stream ended the request first anyway.

        Show
        Mark Miller added a comment - It's the right thing to do. I have read the doc, the code, and mailing list info. I have done tests. After you are done using a connection you need to do either two things: abort it or close the content stream. If you abort it, you won't reuse the connection. There is no need to read through the whole inputstream. There is no need to use abort in normal safe code unless you need that as a feature to interrupt a request. All the proper example code shows the pattern to use is to just close the content input stream. In the end, it's really simple to test by hand and see what causes connection reuse and what breaks it. It was a mistake to add the abort call, but luckily it never had an effect - closing the content stream ended the request first anyway.
        Hide
        Mark Miller added a comment -

        I have a connection reuse test that hits HttpSolrClient, CloudSolrClient, and ConcurrentUpdateSolrClient. Once I polish it up a little, I'll commit it with this issue.

        Show
        Mark Miller added a comment - I have a connection reuse test that hits HttpSolrClient, CloudSolrClient, and ConcurrentUpdateSolrClient. Once I polish it up a little, I'll commit it with this issue.
        Hide
        Mark Miller added a comment -

        Patch with connection reuse test attached. This new test won't work until we address the troublesome Jetty upgrade.

        Show
        Mark Miller added a comment - Patch with connection reuse test attached. This new test won't work until we address the troublesome Jetty upgrade.
        Hide
        ASF subversion and git services added a comment -

        Commit 1723615 from Mark Miller in branch 'dev/trunk'
        [ https://svn.apache.org/r1723615 ]

        SOLR-8451: We should not call method.abort in HttpSolrClient or HttpSolrCall#remoteQuery and HttpSolrCall#remoteQuery should not close streams.

        Show
        ASF subversion and git services added a comment - Commit 1723615 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1723615 ] SOLR-8451 : We should not call method.abort in HttpSolrClient or HttpSolrCall#remoteQuery and HttpSolrCall#remoteQuery should not close streams.
        Hide
        ASF subversion and git services added a comment -

        Commit 1724516 from Mark Miller in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1724516 ]

        SOLR-8451: We should not call method.abort in HttpSolrClient or HttpSolrCall#remoteQuery and HttpSolrCall#remoteQuery should not close streams.

        Show
        ASF subversion and git services added a comment - Commit 1724516 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1724516 ] SOLR-8451 : We should not call method.abort in HttpSolrClient or HttpSolrCall#remoteQuery and HttpSolrCall#remoteQuery should not close streams.
        Hide
        Mark Miller added a comment -

        Okay, I'd like to come back to this and actually add an attempt to make sure we have always consumed the full response inputstream before closing it.

        I have read, and testing shows, HttpClient appears to clean up these situations for us. We have fine connection reuse, even under error conditions. However, given Jetty was doing this for us on the server side basically, and then all of a sudden it was only half hearted attempting it (which broke us), I think it's better to put it into our client code explicitly.

        Just would make me sleep better, especially considering issues like SOLR-8578 ConcurrentUpdateSolrServer with BinaryResponseParser leaves 40-42 bytes unconsumed from the request response even without errors.

        Show
        Mark Miller added a comment - Okay, I'd like to come back to this and actually add an attempt to make sure we have always consumed the full response inputstream before closing it. I have read, and testing shows, HttpClient appears to clean up these situations for us. We have fine connection reuse, even under error conditions. However, given Jetty was doing this for us on the server side basically, and then all of a sudden it was only half hearted attempting it (which broke us), I think it's better to put it into our client code explicitly. Just would make me sleep better, especially considering issues like SOLR-8578 ConcurrentUpdateSolrServer with BinaryResponseParser leaves 40-42 bytes unconsumed from the request response even without errors.
        Hide
        Mark Miller added a comment -

        HttpClient appears to clean up these situations for us.

        I have not been able to find it in the code. Perhaps it's java itself actually:

        Prior to JDK 6, if an application closes a HTTP InputStream when more than a small amount of data remains to be read, then the connection had to be closed, rather than being cached. Now in JDK 6, the behavior is to read up to 512 Kbytes off the connection in a background thread, thus allowing the connection to be reused. The exact amount of data which may be read is configurable through the http.KeepAlive.remainingData system property.
        

        All the more reason to do it right our self if so.

        It may be the case that simply closing the inputstream is all we need to do:

        if the response body is long and you are not interested in the rest of it after seeing the beginning, you can close the InputStream.

        But, that may usually work, but not always:

        But you need to be aware that more data could be on its way. Thus the connection may not be cleared for reuse.

        So I see great connection reuse in tests, but this is all hairy stuff. We need to consume these streams just to be sure we are hitting every case.

        Show
        Mark Miller added a comment - HttpClient appears to clean up these situations for us. I have not been able to find it in the code. Perhaps it's java itself actually: Prior to JDK 6, if an application closes a HTTP InputStream when more than a small amount of data remains to be read, then the connection had to be closed, rather than being cached. Now in JDK 6, the behavior is to read up to 512 Kbytes off the connection in a background thread, thus allowing the connection to be reused. The exact amount of data which may be read is configurable through the http.KeepAlive.remainingData system property. All the more reason to do it right our self if so. It may be the case that simply closing the inputstream is all we need to do: if the response body is long and you are not interested in the rest of it after seeing the beginning, you can close the InputStream. But, that may usually work, but not always: But you need to be aware that more data could be on its way. Thus the connection may not be cleared for reuse. So I see great connection reuse in tests, but this is all hairy stuff. We need to consume these streams just to be sure we are hitting every case.
        Hide
        ASF subversion and git services added a comment -

        Commit 62c9b6a1724fb466fd767457b7bbdeb24e164036 in lucene-solr's branch refs/heads/master from Mark Miller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=62c9b6a ]

        SOLR-8451: fix randomization in test.

        Show
        ASF subversion and git services added a comment - Commit 62c9b6a1724fb466fd767457b7bbdeb24e164036 in lucene-solr's branch refs/heads/master from Mark Miller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=62c9b6a ] SOLR-8451 : fix randomization in test.
        Hide
        ASF subversion and git services added a comment -

        Commit 93172e461f76816474c1a1f2be8118c4b5e8538e in lucene-solr's branch refs/heads/branch_5x from Mark Miller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=93172e4 ]

        SOLR-8451: fix randomization in test.

        Show
        ASF subversion and git services added a comment - Commit 93172e461f76816474c1a1f2be8118c4b5e8538e in lucene-solr's branch refs/heads/branch_5x from Mark Miller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=93172e4 ] SOLR-8451 : fix randomization in test.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development