Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-11692

SolrDispatchFilter.closeShield passes the shielded response object back to jetty making the stream unclose able

Details

    Description

      In test mode we trigger closeShield code in SolrDispatchFilter, however there are code paths where we passthrough the objects to the DefaultHandler which can no longer close the response.

      Example stack trace:
      java.lang.AssertionError: Attempted close of response output stream.
      at org.apache.solr.servlet.SolrDispatchFilter$2$1.close(SolrDispatchFilter.java:528)
      at org.eclipse.jetty.server.Dispatcher.commitResponse(Dispatcher.java:315)
      at org.eclipse.jetty.server.Dispatcher.forward(Dispatcher.java:279)
      at org.eclipse.jetty.server.Dispatcher.forward(Dispatcher.java:103)
      at org.eclipse.jetty.servlet.DefaultServlet.doGet(DefaultServlet.java:566)
      at javax.servlet.http.HttpServlet.service(HttpServlet.java:734)
      at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)
      at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:684)
      at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1448)
      at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:385)
      at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:326)
      at searchserver.filter.SfdcDispatchFilter.doFilter(SfdcDispatchFilter.java:204)
      at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1419)
      at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:455)
      at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:137)
      at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:557)
      at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:231)
      at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1075)
      at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:384)
      at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:193)
      at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1009)
      at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:135)
      at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:255)
      at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:154)
      at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:116)
      at org.eclipse.jetty.server.Server.handle(Server.java:370)
      at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:489)
      at org.eclipse.jetty.server.AbstractHttpConnection.headerComplete(AbstractHttpConnection.java:949)
      at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.headerComplete(AbstractHttpConnection.java:1011)
      at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:644)
      at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:235)
      at org.eclipse.jetty.server.AsyncHttpConnection.handle(AsyncHttpConnection.java:82)
      at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:668)
      at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:52)
      at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:608)
      at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:543)
      at java.lang.Thread.run(Thread.java:745)

      Related JIRA: SOLR-8933

      Attachments

        1. SOLR-11692.patch
          1 kB
          Jeff Miller
        2. SOLR-11692.patch
          8 kB
          David Smiley

        Issue Links

          Activity

            millerjeff0 Jeff Miller added a comment - - edited

            Note this is test mode only code, doesn't affect solr with asserts turned off. dsmiley suggested a fix where we keep the original response object to pass back up to jetty when solr code is done with it, I will send out a attempted patch later

            millerjeff0 Jeff Miller added a comment - - edited Note this is test mode only code, doesn't affect solr with asserts turned off. dsmiley suggested a fix where we keep the original response object to pass back up to jetty when solr code is done with it, I will send out a attempted patch later
            millerjeff0 Jeff Miller added a comment - - edited

            markrmiller@gmail.com Can you comment on this patch? The idea being we wrap the closeshield for the request/response only in the context of SolrDispatchFilter and if we have to pass it up to chain or forward it we pass the original

            <Removed Pasted Code>

            millerjeff0 Jeff Miller added a comment - - edited markrmiller@gmail.com Can you comment on this patch? The idea being we wrap the closeshield for the request/response only in the context of SolrDispatchFilter and if we have to pass it up to chain or forward it we pass the original <Removed Pasted Code>
            dsmiley David Smiley added a comment -

            millerjeff0 can you attach a patch to the issue instead? inlining diffs is problematic due to escaping. Also, unlike some diffs which need no context, for this one it's necessary to apply it to notice that your intent was to move the closing until after code that calls chain.doFilter. I looked at SolrDispatchFilter and do note chain.doFilter is actually in two places (not one); the second is beyond where you moved it to. So I think your patch here only addresses the issue for some cases but not others. Any way, the fix should be easy.

            dsmiley David Smiley added a comment - millerjeff0 can you attach a patch to the issue instead? inlining diffs is problematic due to escaping. Also, unlike some diffs which need no context, for this one it's necessary to apply it to notice that your intent was to move the closing until after code that calls chain.doFilter . I looked at SolrDispatchFilter and do note chain.doFilter is actually in two places (not one); the second is beyond where you moved it to. So I think your patch here only addresses the issue for some cases but not others. Any way, the fix should be easy.
            millerjeff0 Jeff Miller added a comment -

            Thanks dsmiley, here is a patch

            millerjeff0 Jeff Miller added a comment - Thanks dsmiley , here is a patch
            dsmiley David Smiley added a comment -

            +1 I like it Jeff.

            The only change I suggest making is a little bit of maintenance here regarding the annoying casting of ServletRequest to HttpServletRequest in several places in this method. You've added another spot. Notice the first line of this method ensures we have the HTTP version. Perhaps we can rename the parameter slightly and then cast to the current name in a local variable... then change various methods here to expect/return HttpServletRequest. What do you think?

            dsmiley David Smiley added a comment - +1 I like it Jeff. The only change I suggest making is a little bit of maintenance here regarding the annoying casting of ServletRequest to HttpServletRequest in several places in this method. You've added another spot. Notice the first line of this method ensures we have the HTTP version. Perhaps we can rename the parameter slightly and then cast to the current name in a local variable... then change various methods here to expect/return HttpServletRequest. What do you think?
            millerjeff0 Jeff Miller added a comment - dsmiley sent PR https://github.com/apache/lucene-solr/pull/281
            dsmiley David Smiley added a comment -

            Ok; when creating PRs, be sure to reference the JIRA issue ID so that it gets linked automatically. Maybe PRs can be renamed? If not; no big deal.

            BTW thanks for entertaining my request to do this little bit of cleanup. IMO cleanup needs to be amortized into all we do; doing cleanup just for cleanup sake (its own issue) is hard to get the time for.

            dsmiley David Smiley added a comment - Ok; when creating PRs, be sure to reference the JIRA issue ID so that it gets linked automatically. Maybe PRs can be renamed? If not; no big deal. BTW thanks for entertaining my request to do this little bit of cleanup. IMO cleanup needs to be amortized into all we do; doing cleanup just for cleanup sake (its own issue) is hard to get the time for.
            dsmiley David Smiley added a comment -

            Code looks good – I'm running test now and will commit after. Thanks for contributing Jeff!

            Proposed CHANGES.txt

            Other Changes:
            * SOLR-11692: (tests only) When SolrDispatchFilter passes on requests in the Servlet filter chain, 
              it should not use its closeShield'ed streams.  (Jeff Miller, David Smiley)
            
            dsmiley David Smiley added a comment - Code looks good – I'm running test now and will commit after. Thanks for contributing Jeff! Proposed CHANGES.txt Other Changes: * SOLR-11692: (tests only) When SolrDispatchFilter passes on requests in the Servlet filter chain, it should not use its closeShield'ed streams. (Jeff Miller, David Smiley)
            dsmiley David Smiley added a comment -

            4 tests fail, and I tried at at least some of them with and without the patch to verify they reproduced with the patch but not without the patch:

               [junit4] Tests with failures [seed: 37DAB2BA08A37C95]:
               [junit4]   - org.apache.solr.security.BasicAuthStandaloneTest.testBasicAuth
               [junit4]   - org.apache.solr.cloud.TestConfigSetsAPI.testUploadWithScriptUpdateProcessor
               [junit4]   - org.apache.solr.security.BasicAuthIntegrationTest.testBasicAuth
               [junit4]   - org.apache.solr.security.PKIAuthenticationIntegrationTest.testPkiAuth
            
            dsmiley David Smiley added a comment - 4 tests fail, and I tried at at least some of them with and without the patch to verify they reproduced with the patch but not without the patch: [junit4] Tests with failures [seed: 37DAB2BA08A37C95]: [junit4] - org.apache.solr.security.BasicAuthStandaloneTest.testBasicAuth [junit4] - org.apache.solr.cloud.TestConfigSetsAPI.testUploadWithScriptUpdateProcessor [junit4] - org.apache.solr.security.BasicAuthIntegrationTest.testBasicAuth [junit4] - org.apache.solr.security.PKIAuthenticationIntegrationTest.testPkiAuth
            dsmiley David Smiley added a comment -

            Aha; I see the problem. Took a bit of debugging to find this nasty bug. Notice on line ~353 that the request var is replaced with wrappedRequest.get() if there's something there. In your patch you retained both "request" and "httpServletRequest" as valid variables... which is conducive to causing this bug. In this new version of the patch, I renamed the formal parameter names to have a leading underscore and then first thing I immediately assign them to the Http variant with the same name without the leading underscore.

            Tests are running now; if it checks out I'll commit shortly. Thanks for the contribution Jeff!

            dsmiley David Smiley added a comment - Aha; I see the problem. Took a bit of debugging to find this nasty bug. Notice on line ~353 that the request var is replaced with wrappedRequest.get() if there's something there. In your patch you retained both "request" and "httpServletRequest" as valid variables... which is conducive to causing this bug. In this new version of the patch, I renamed the formal parameter names to have a leading underscore and then first thing I immediately assign them to the Http variant with the same name without the leading underscore. Tests are running now; if it checks out I'll commit shortly. Thanks for the contribution Jeff!

            Commit 7a375fda828015ab62702e2e0f07a1038aef40c6 in lucene-solr's branch refs/heads/master from dsmiley
            [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a375fd ]

            SOLR-11692: Constrain cases where SolrDispatchFilter uses closeShield

            jira-bot ASF subversion and git services added a comment - Commit 7a375fda828015ab62702e2e0f07a1038aef40c6 in lucene-solr's branch refs/heads/master from dsmiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a375fd ] SOLR-11692 : Constrain cases where SolrDispatchFilter uses closeShield

            Commit 9e3c16cf2ef0d2b9d562d8df75b4688d6ce0131a in lucene-solr's branch refs/heads/branch_7x from dsmiley
            [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9e3c16c ]

            SOLR-11692: Constrain cases where SolrDispatchFilter uses closeShield

            (cherry picked from commit 7a375fd)

            jira-bot ASF subversion and git services added a comment - Commit 9e3c16cf2ef0d2b9d562d8df75b4688d6ce0131a in lucene-solr's branch refs/heads/branch_7x from dsmiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9e3c16c ] SOLR-11692 : Constrain cases where SolrDispatchFilter uses closeShield (cherry picked from commit 7a375fd)
            githubbot ASF GitHub Bot added a comment -

            Github user tflobbe commented on the issue:

            https://github.com/apache/lucene-solr/pull/281

            This PR was already merged (SOLR-11692) but wasn't mentioned in the commit message. Could you close @millerjeff0 ?

            githubbot ASF GitHub Bot added a comment - Github user tflobbe commented on the issue: https://github.com/apache/lucene-solr/pull/281 This PR was already merged ( SOLR-11692 ) but wasn't mentioned in the commit message. Could you close @millerjeff0 ?
            markrmiller@gmail.com Mark Miller added a comment -

            Sorry, I missed this. The jetty servlet actually should not be closing in forward like that, we need to reuse our connections and only the container itself should close them. I've addressed this properly and expanded shielding to cover even more than it was in SOLR-12290.

            markrmiller@gmail.com Mark Miller added a comment - Sorry, I missed this. The jetty servlet actually should not be closing in forward like that, we need to reuse our connections and only the container itself should close them. I've addressed this properly and expanded shielding to cover even more than it was in SOLR-12290 .

            People

              dsmiley David Smiley
              millerjeff0 Jeff Miller
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

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