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

SolrDispatchFilter::consumeInput logs "Stream Closed" IOException

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 4.10.3
    • Fix Version/s: 5.5.2, 5.6, 6.0.2, 6.1, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      After SOLR-8453 we started seeing some IOExceptions coming out of SolrDispatchFilter with "Stream Closed" messages.

      It looks like we are indeed closing the request stream in several places when we really need to be letting the web container handle their life cycle. I've got a preliminary patch ready and am working on testing it to make sure there are no regressions.

      A very strange piece of this is that I have been entirely unable to reproduce it on a unit test, but have seen it on cluster deployment quite consistently.

      1. SOLR-8933.patch
        23 kB
        Mark Miller
      2. SOLR-8933.patch
        21 kB
        Mike Drob
      3. SOLR-8933.patch
        19 kB
        Mike Drob
      4. SOLR-8933.patch
        19 kB
        Mike Drob
      5. SOLR-8933.patch
        8 kB
        Mike Drob
      6. SOLR-8933.patch
        23 kB
        Mike Drob
      7. SOLR-8933.patch
        23 kB
        Mike Drob
      8. SOLR-8933.patch
        17 kB
        Mike Drob
      9. SOLR-8933.patch
        17 kB
        Mike Drob
      10. SOLR-8933.patch
        11 kB
        Mike Drob
      11. SOLR-8933.patch
        11 kB
        Mike Drob

        Activity

        Hide
        mdrob Mike Drob added a comment -

        Attaching a trunk patch.

        Show
        mdrob Mike Drob added a comment - Attaching a trunk patch.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        SingleThreadedJsonLoader looks like it's off?

              try {
                // We won't close this stream because it belongs to ServletContainer Request
                Reader reader = stream.getReader();
                if (log.isTraceEnabled()) {
                  String body = IOUtils.toString(reader);
                  log.trace("body", body);
                  reader = new StringReader(body);
                }
        
                this.processUpdate(reader);
              } catch (ParseException e) {
                throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Cannot parse provided JSON: " + e.getMessage());
              }
              
              parser = new JSONParser(reader);
              this.processUpdate();
            }
        
        Show
        markrmiller@gmail.com Mark Miller added a comment - SingleThreadedJsonLoader looks like it's off? try { // We won't close this stream because it belongs to ServletContainer Request Reader reader = stream.getReader(); if (log.isTraceEnabled()) { String body = IOUtils.toString(reader); log.trace( "body" , body); reader = new StringReader(body); } this .processUpdate(reader); } catch (ParseException e) { throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Cannot parse provided JSON: " + e.getMessage()); } parser = new JSONParser(reader); this .processUpdate(); }
        Hide
        mdrob Mike Drob added a comment -

        Thanks for catching that, I had started with an unclean workspace. Here's an updated patch.

        Show
        mdrob Mike Drob added a comment - Thanks for catching that, I had started with an unclean workspace. Here's an updated patch.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Can we clear up the report a bit? I'm kind of confused as it is.

        SolrDispatchFilter::consumeInput logs "Stream Closed" IOException

        Have we ever actually seen that happen with Jetty or on trunk, or have we still only seen it with a version of 4.10.3 and Tomcat?

        A very strange piece of this is that I have been entirely unable to reproduce it on a unit test, but have seen it on cluster deployment quite consistently.

        Same question - a unit test on 4.10.3 version or trunk or both? Same with the the cluster deployment? Is that older? Tomcat or Jetty?

        Just trying to understand when we are talking about our 4.10.3 version, latest trunk, and Tomcat vs Jetty.

        Last I had seen, while we don't want to close these streams early regardless, on trunk with Jetty, it doesn't actually log anything or cause us any grief.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Can we clear up the report a bit? I'm kind of confused as it is. SolrDispatchFilter::consumeInput logs "Stream Closed" IOException Have we ever actually seen that happen with Jetty or on trunk, or have we still only seen it with a version of 4.10.3 and Tomcat? A very strange piece of this is that I have been entirely unable to reproduce it on a unit test, but have seen it on cluster deployment quite consistently. Same question - a unit test on 4.10.3 version or trunk or both? Same with the the cluster deployment? Is that older? Tomcat or Jetty? Just trying to understand when we are talking about our 4.10.3 version, latest trunk, and Tomcat vs Jetty. Last I had seen, while we don't want to close these streams early regardless, on trunk with Jetty, it doesn't actually log anything or cause us any grief.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Also, are we sure this patch catches all the spots this can happen on trunk? How did you track down each spot the streams are closed early?

        Is there any chance this can be added to forbidden APIs? Not sure offhand if it can actually cover this case. Uwe Schindler?

        Show
        markrmiller@gmail.com Mark Miller added a comment - Also, are we sure this patch catches all the spots this can happen on trunk? How did you track down each spot the streams are closed early? Is there any chance this can be added to forbidden APIs? Not sure offhand if it can actually cover this case. Uwe Schindler ?
        Hide
        thetaphi Uwe Schindler added a comment -

        Hi,
        Unfortunately you cannot prevent closing with forbiddenApis, because it would prevent all Closeable.close calls.

        My tip is to wrap the stream with a wrapper that intercepts close() in the dispatch filter. Lots of webapps are doing this.

        In my personal opinion, the server container should take care of preventing the close.

        Ioutils has a wrapper for this!

        Show
        thetaphi Uwe Schindler added a comment - Hi, Unfortunately you cannot prevent closing with forbiddenApis, because it would prevent all Closeable.close calls. My tip is to wrap the stream with a wrapper that intercepts close() in the dispatch filter. Lots of webapps are doing this. In my personal opinion, the server container should take care of preventing the close. Ioutils has a wrapper for this!
        Hide
        mdrob Mike Drob added a comment -

        Have we ever actually seen that happen with Jetty or on trunk, or have we still only seen it with a version of 4.10.3 and Tomcat?

        I've only seen it on 4.10.3 and Tomcat. As Uwe Schindler alludes to later, Jetty might be taking care of this for us.

        Also, are we sure this patch catches all the spots this can happen on trunk? How did you track down each spot the streams are closed early?

        I wrapped the request and intercepted requests to close() and logged them with stack traces. I had removed this for the patch because I wasn't confident in the performance impact that I would be causing, but can easily add it back.

        Show
        mdrob Mike Drob added a comment - Have we ever actually seen that happen with Jetty or on trunk, or have we still only seen it with a version of 4.10.3 and Tomcat? I've only seen it on 4.10.3 and Tomcat. As Uwe Schindler alludes to later, Jetty might be taking care of this for us. Also, are we sure this patch catches all the spots this can happen on trunk? How did you track down each spot the streams are closed early? I wrapped the request and intercepted requests to close() and logged them with stack traces. I had removed this for the patch because I wasn't confident in the performance impact that I would be causing, but can easily add it back.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I've only seen it on 4.10.3 and Tomcat. As Uwe Schindler alludes to later, Jetty might be taking care of this for us.

        Okay. Just want anyone reading this issue to be able to understand whether and how it might impact them. I still had not seen this have any effect with Jetty here.

        I wrapped the request and intercepted requests to close() and logged them with stack traces. I had removed this for the patch because I wasn't confident in the performance impact that I would be causing, but can easily add it back.

        I don't think we need it committed, just how you went about it documented. Since we can't forbid this from coming into the code again, we at least want to make this easy on the next person who has to tackle it by getting this info in the ticket.

        Trunk and 6x will always diverge some too, so probably also good to note specifically which branch(s) has been cleared.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I've only seen it on 4.10.3 and Tomcat. As Uwe Schindler alludes to later, Jetty might be taking care of this for us. Okay. Just want anyone reading this issue to be able to understand whether and how it might impact them. I still had not seen this have any effect with Jetty here. I wrapped the request and intercepted requests to close() and logged them with stack traces. I had removed this for the patch because I wasn't confident in the performance impact that I would be causing, but can easily add it back. I don't think we need it committed, just how you went about it documented. Since we can't forbid this from coming into the code again, we at least want to make this easy on the next person who has to tackle it by getting this info in the ticket. Trunk and 6x will always diverge some too, so probably also good to note specifically which branch(s) has been cleared.
        Hide
        mdrob Mike Drob added a comment -

        I could put in the wrapping and instrumentation, and then hide it behind a flag. I'll look at doing that and upload a new patch.

        Show
        mdrob Mike Drob added a comment - I could put in the wrapping and instrumentation, and then hide it behind a flag. I'll look at doing that and upload a new patch.
        Hide
        mdrob Mike Drob added a comment -

        Did some further research on why this error doesn't show up with Jetty. It appears that HttpInput::close() (the jetty-server implementation of ServletInputStream) is a no-op and takes care of the clean-up by calling recycle at some later point. Unsure if it's worth the effort to be technically correct without producing any impact otherwise.

        Show
        mdrob Mike Drob added a comment - Did some further research on why this error doesn't show up with Jetty. It appears that HttpInput::close() (the jetty-server implementation of ServletInputStream) is a no-op and takes care of the clean-up by calling recycle at some later point. Unsure if it's worth the effort to be technically correct without producing any impact otherwise.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Unsure if it's worth the effort to be technically correct

        I think we want to be technically correct here - we don't want to have to count on one specific impl protecting us from ourselves.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Unsure if it's worth the effort to be technically correct I think we want to be technically correct here - we don't want to have to count on one specific impl protecting us from ourselves.
        Hide
        mdrob Mike Drob added a comment -

        Alright, here's another patch that adds the instrumentation hidden behind a system property.

        Show
        mdrob Mike Drob added a comment - Alright, here's another patch that adds the instrumentation hidden behind a system property.
        Hide
        thetaphi Uwe Schindler added a comment -

        Why not just enable it during "test mode". So when a test executes, the wrapper is added. This can be done by a sysproperty, but we have one already: jetty.testmode or like that, which is set during running tests.

        Show
        thetaphi Uwe Schindler added a comment - Why not just enable it during "test mode". So when a test executes, the wrapper is added. This can be done by a sysproperty, but we have one already: jetty.testmode or like that, which is set during running tests.
        Hide
        mdrob Mike Drob added a comment -

        Ioutils has a wrapper for this!

        Which IOUtils? I didn't see this in o.a.lucene.util or o.a.solr.common.util. I'm currently adding a new class CloseShieldServletInputStream inspired by the Apache commons-io CloseShieldInputStream which was almost good enough to use. There's maybe some clever way to handle this via generics, reflection, and proxies, but I think that suffers in terms of maintainability.

        Why not just enable it during "test mode"

        Done. Originally I thought there was value in fine grained control to enable the warning, but at this point I'm not convinced that is true.

        Show
        mdrob Mike Drob added a comment - Ioutils has a wrapper for this! Which IOUtils? I didn't see this in o.a.lucene.util or o.a.solr.common.util . I'm currently adding a new class CloseShieldServletInputStream inspired by the Apache commons-io CloseShieldInputStream which was almost good enough to use. There's maybe some clever way to handle this via generics, reflection, and proxies, but I think that suffers in terms of maintainability. Why not just enable it during "test mode" Done. Originally I thought there was value in fine grained control to enable the warning, but at this point I'm not convinced that is true.
        Hide
        thetaphi Uwe Schindler added a comment - - edited

        Which IOUtils? I didn't see this in o.a.lucene.util or o.a.solr.common.util.

        Sorry I was thinking of commons-io instead of IOUtils. Was a typo!

        CloseShieldServletInputStream inspired by the Apache commons-io CloseShieldInputStream which was almost good enough to use.

        I would just use the CloseShieldInputStream. The additional APIs provided by ServletInputStream or ServletOutputStream are not used by Solr. Actually we put some of them to the forbidden-apis list (because of stupidity and ISO-8859-1 defaults from earlier days). See lucene/tools/forbiddenApis/servlet-api.txt

        I looked at this. There is no penalty by the wrapping - so you can also do it by default (with CloseShieldInputStream, no specialization). This is just optimized away... A method that just delegates to another methods is always removed by the VM.

        Show
        thetaphi Uwe Schindler added a comment - - edited Which IOUtils? I didn't see this in o.a.lucene.util or o.a.solr.common.util. Sorry I was thinking of commons-io instead of IOUtils. Was a typo! CloseShieldServletInputStream inspired by the Apache commons-io CloseShieldInputStream which was almost good enough to use. I would just use the CloseShieldInputStream. The additional APIs provided by ServletInputStream or ServletOutputStream are not used by Solr. Actually we put some of them to the forbidden-apis list (because of stupidity and ISO-8859-1 defaults from earlier days). See lucene/tools/forbiddenApis/servlet-api.txt I looked at this. There is no penalty by the wrapping - so you can also do it by default (with CloseShieldInputStream, no specialization). This is just optimized away... A method that just delegates to another methods is always removed by the VM.
        Hide
        mdrob Mike Drob added a comment -

        I would just use the CloseShieldInputStream. The additional APIs provided by ServletInputStream or ServletOutputStream are not used by Solr. Actually we put some of them to the forbidden-apis list (because of stupidity and ISO-8859-1 defaults from earlier days). See lucene/tools/forbiddenApis/servlet-api.txt

        Doesn't matter that we don't use the extra APIs, the type signature of ServletRequest::getInputStream still demands a ServletInputStream so that is what we have to return when wrapping the request object. The other option is to go even deeper down the hierarchy, cast to org.eclipse.jetty.server.Request and then manipulate the input stream directly from there, but that feels much more invasive and brittle.

        Show
        mdrob Mike Drob added a comment - I would just use the CloseShieldInputStream. The additional APIs provided by ServletInputStream or ServletOutputStream are not used by Solr. Actually we put some of them to the forbidden-apis list (because of stupidity and ISO-8859-1 defaults from earlier days). See lucene/tools/forbiddenApis/servlet-api.txt Doesn't matter that we don't use the extra APIs, the type signature of ServletRequest::getInputStream still demands a ServletInputStream so that is what we have to return when wrapping the request object. The other option is to go even deeper down the hierarchy, cast to org.eclipse.jetty.server.Request and then manipulate the input stream directly from there, but that feels much more invasive and brittle.
        Hide
        thetaphi Uwe Schindler added a comment - - edited

        Hi,

        Doesn't matter that we don't use the extra APIs, the type signature of ServletRequest::getInputStream still demands a ServletInputStream so that is what we have to return when wrapping the request object.

        Yeah, I see you are wrapping the whole ServletRequest.

        In any case, I skimmed through the patch: There is one problem. You don't close ContentStream's InputStream anymore. But there are cases in Solr, where the ContentStream does not come from a Request, but from somewhere else: It could also be the "virtual" stream from the URL param (OK this one does not need to be closed), but it could also be a remote stream e.g,, if you pass a stream.url parameter to the servlet. In that case it is never closed.

        This would then be a bug!!! I'd suggest to do the following: Always wrap with CloseShield in DispatchFilter and ignore the close without logging anything (for safety with non-Jetty or later Jetty versions) and revert all other changes in Solr. To me it is unnatural to not close a stream after you have consumed it. I'd also change the consumers to try-with-resources.

        The ContentStream tests are using remote streams (actually a file:// URL for testing). So on Windows, tests should fail with a resource leak there.

        Show
        thetaphi Uwe Schindler added a comment - - edited Hi, Doesn't matter that we don't use the extra APIs, the type signature of ServletRequest::getInputStream still demands a ServletInputStream so that is what we have to return when wrapping the request object. Yeah, I see you are wrapping the whole ServletRequest. In any case, I skimmed through the patch: There is one problem. You don't close ContentStream's InputStream anymore. But there are cases in Solr, where the ContentStream does not come from a Request, but from somewhere else: It could also be the "virtual" stream from the URL param (OK this one does not need to be closed), but it could also be a remote stream e.g,, if you pass a stream.url parameter to the servlet. In that case it is never closed. This would then be a bug!!! I'd suggest to do the following: Always wrap with CloseShield in DispatchFilter and ignore the close without logging anything (for safety with non-Jetty or later Jetty versions) and revert all other changes in Solr. To me it is unnatural to not close a stream after you have consumed it. I'd also change the consumers to try-with-resources. The ContentStream tests are using remote streams (actually a file:// URL for testing). So on Windows, tests should fail with a resource leak there.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        To me it is unnatural to not close a stream after you have consumed it.

        I don't like closing it at all. I think in java it is natural - you close the streams you create, not the open streams that are passed to you.

        Show
        markrmiller@gmail.com Mark Miller added a comment - To me it is unnatural to not close a stream after you have consumed it. I don't like closing it at all. I think in java it is natural - you close the streams you create, not the open streams that are passed to you.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        You don't close ContentStream's InputStream anymore.

        That sounds like perhaps ContentStream should be closed and understand more about the lifecycle of the inputstream it's using so that only streams it owns are closed.

        It really seems strange to count on this Jetty impl detail and close streams we don't own.

        Show
        markrmiller@gmail.com Mark Miller added a comment - You don't close ContentStream's InputStream anymore. That sounds like perhaps ContentStream should be closed and understand more about the lifecycle of the inputstream it's using so that only streams it owns are closed. It really seems strange to count on this Jetty impl detail and close streams we don't own.
        Hide
        mdrob Mike Drob added a comment -

        The ContentStream tests are using remote streams (actually a file:// URL for testing). So on Windows, tests should fail with a resource leak there.

        The cases in ContentStreamTest are still closing everything. I don't understand what the relation there is. I haven't tested with Windows though, so maybe there is additional nuance I am missing.

        I found the part where we are processing remote stream parameter to set the content stream... Maybe it makes sense to make ContentStream implement Closeable and then we can know to close the URL and File streams but not the Servlet, Byte, or String streams. They're all build from the Request in SolrRequestParsers::parse - at which point we can make that determination.

        The next question becomes then, who is responsible for closing the ContentStream? It's easy to make the SolrRequest responsible for the ContentStreams it creates, but I am not sure if there are other cases. And there are too many warnings in the project for me to reliably tell what I'm missing.

        Here's a proposed patch heading down this path, likely incomplete.

        Show
        mdrob Mike Drob added a comment - The ContentStream tests are using remote streams (actually a file:// URL for testing). So on Windows, tests should fail with a resource leak there. The cases in ContentStreamTest are still closing everything. I don't understand what the relation there is. I haven't tested with Windows though, so maybe there is additional nuance I am missing. I found the part where we are processing remote stream parameter to set the content stream... Maybe it makes sense to make ContentStream implement Closeable and then we can know to close the URL and File streams but not the Servlet, Byte, or String streams. They're all build from the Request in SolrRequestParsers::parse - at which point we can make that determination. The next question becomes then, who is responsible for closing the ContentStream? It's easy to make the SolrRequest responsible for the ContentStreams it creates, but I am not sure if there are other cases. And there are too many warnings in the project for me to reliably tell what I'm missing. Here's a proposed patch heading down this path, likely incomplete.
        Hide
        mdrob Mike Drob added a comment -

        A more thorough patch. I think I got everything in this one, but the usages I tracked down were all found via eclipse 'find usage.' There might be more.

        Show
        mdrob Mike Drob added a comment - A more thorough patch. I think I got everything in this one, but the usages I tracked down were all found via eclipse 'find usage.' There might be more.
        Hide
        mdrob Mike Drob added a comment -

        So it occurred to me that we might be able to get away with wrapping the HttpRequestContentStream in a CloseShieldInputStream for minimal changes, assuming that everything else in the code base behaves consistently by reading the content stream and not the stream from the request, and then let various handlers/parsers/etc. still try to close the stream when they are done.

        Show
        mdrob Mike Drob added a comment - So it occurred to me that we might be able to get away with wrapping the HttpRequestContentStream in a CloseShieldInputStream for minimal changes, assuming that everything else in the code base behaves consistently by reading the content stream and not the stream from the request, and then let various handlers/parsers/etc. still try to close the stream when they are done.
        Hide
        mdrob Mike Drob added a comment -

        Attaching a much simpler patch that I think still addresses the underlying issue of relying on Jetty's no-op close impl.

        Show
        mdrob Mike Drob added a comment - Attaching a much simpler patch that I think still addresses the underlying issue of relying on Jetty's no-op close impl.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        What about the outputstream? We should give that the same treatment.

        Show
        markrmiller@gmail.com Mark Miller added a comment - What about the outputstream? We should give that the same treatment.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        ServletInputStreamWrapper

        Let's add a class comment that mentions why this exists.

        + if (System.getProperty("jetty.testMode") != null && !retry) {

        I really don't like introducing any memory barriers for this at such an important code path. I wonder if we could use an assert and private method somehow.

        It seems that even if it works, it's less than ideal to make it a static member and count on initialize vs setting the test sys prop order, but perhaps that is an option as well?

        new CloseShieldInputStream

        I think it's worth adding a small comment when we use this that says something like: protect container owned streams from being closed by Solr, see SOLR-8933

        log.warn("", new IOException("Attempted close of request input stream."));

        Since this is test logging, perhaps it should be ERROR so that violators feel worse about it if they see the logging. Warnings are easy to ignore.

        Show
        markrmiller@gmail.com Mark Miller added a comment - ServletInputStreamWrapper Let's add a class comment that mentions why this exists. + if (System.getProperty("jetty.testMode") != null && !retry) { I really don't like introducing any memory barriers for this at such an important code path. I wonder if we could use an assert and private method somehow. It seems that even if it works, it's less than ideal to make it a static member and count on initialize vs setting the test sys prop order, but perhaps that is an option as well? new CloseShieldInputStream I think it's worth adding a small comment when we use this that says something like: protect container owned streams from being closed by Solr, see SOLR-8933 log.warn("", new IOException("Attempted close of request input stream.")); Since this is test logging, perhaps it should be ERROR so that violators feel worse about it if they see the logging. Warnings are easy to ignore.
        Hide
        mdrob Mike Drob added a comment -

        Patch that adds docs and provides the same treatment to (most) output streams.

        I decided to leave the log messages at warn since even if they appear, the problem has been largely mitigated away already.

        Show
        mdrob Mike Drob added a comment - Patch that adds docs and provides the same treatment to (most) output streams. I decided to leave the log messages at warn since even if they appear, the problem has been largely mitigated away already.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        the problem has been largely mitigated away already.

        Then there should be no reason to introduce this test code path into the production code.

        I think there is good reason though - to make sure users of this API use it properly. If we are going to try and fix this at all, we want to maintain it rather than reverting to counting on Jetties internal impl. That is what the test mode code is all about. So I think Error is more appropriate than INFO, but actually...

        Which makes me think, actually, more than logging at Error level, we should probably fail an assert there and prevent tests from passing. We are looking to enforce this if we can - a logging indicator only would be worst case I think.

        We want the build to fail when someone adds some new code and it does not use a shield but should.

        Show
        markrmiller@gmail.com Mark Miller added a comment - the problem has been largely mitigated away already. Then there should be no reason to introduce this test code path into the production code. I think there is good reason though - to make sure users of this API use it properly. If we are going to try and fix this at all, we want to maintain it rather than reverting to counting on Jetties internal impl. That is what the test mode code is all about. So I think Error is more appropriate than INFO, but actually... Which makes me think, actually, more than logging at Error level, we should probably fail an assert there and prevent tests from passing. We are looking to enforce this if we can - a logging indicator only would be worst case I think. We want the build to fail when someone adds some new code and it does not use a shield but should.
        Hide
        mdrob Mike Drob added a comment -

        Which makes me think, actually, more than logging at Error level, we should probably fail an assert there and prevent tests from passing. We are looking to enforce this if we can - a logging indicator only would be worst case I think.

        Yes! This makes sense to me, now that you bring it up. I'm not used to using asserts on other projects, but they make sense here.

        Attaching a patch that uses asserts instead of logging. (Should it do both, on the off chance that testMode is enabled but asserts are disabled?)

        I have not run the full test suite on this yet, I'll let it go overnight and see what shakes out, but wanted to get the patch up for more eyes first.

        Show
        mdrob Mike Drob added a comment - Which makes me think, actually, more than logging at Error level, we should probably fail an assert there and prevent tests from passing. We are looking to enforce this if we can - a logging indicator only would be worst case I think. Yes! This makes sense to me, now that you bring it up. I'm not used to using asserts on other projects, but they make sense here. Attaching a patch that uses asserts instead of logging. (Should it do both, on the off chance that testMode is enabled but asserts are disabled?) I have not run the full test suite on this yet, I'll let it go overnight and see what shakes out, but wanted to get the patch up for more eyes first.
        Hide
        mdrob Mike Drob added a comment -

        Ran the tests, most of them looked fine, there were a few that failed but passed when I reran them so I didn't take too much note.

        Uwe Schindler, Mark Miller - what do y'all think?

        Show
        mdrob Mike Drob added a comment - Ran the tests, most of them looked fine, there were a few that failed but passed when I reran them so I didn't take too much note. Uwe Schindler , Mark Miller - what do y'all think?
        Hide
        thetaphi Uwe Schindler added a comment -

        Hi,
        I am fine with the way how it is handles by SolrRequestParsers. This was my only complaint.

        I do not take care if we should rely on streams not closed or keep them open, I was just afraid of resource leaks because ContentStreams were not closed anymore in earlier patches. But this is perfectly solved by commons-io CloseShield just inside SolrRequestParsers. For everything else (preventing closing of the underlying ServletStreams) inside SolrDispatchFilter: I have no strong preference. But code looks fine. I leave that open to You and Mark how to handle this..

        Show
        thetaphi Uwe Schindler added a comment - Hi, I am fine with the way how it is handles by SolrRequestParsers. This was my only complaint. I do not take care if we should rely on streams not closed or keep them open, I was just afraid of resource leaks because ContentStreams were not closed anymore in earlier patches. But this is perfectly solved by commons-io CloseShield just inside SolrRequestParsers. For everything else (preventing closing of the underlying ServletStreams) inside SolrDispatchFilter: I have no strong preference. But code looks fine. I leave that open to You and Mark how to handle this..
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I see some tests now fail due to the new assert. Seems there are some closes we don't shield still.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I see some tests now fail due to the new assert. Seems there are some closes we don't shield still.
        Hide
        mdrob Mike Drob added a comment -

        I thought I got all of the test failures previously, looks like I missed two usages though. Pretty confident that I got them this time, thanks for verifying as well, Mark Miller

        Show
        mdrob Mike Drob added a comment - I thought I got all of the test failures previously, looks like I missed two usages though. Pretty confident that I got them this time, thanks for verifying as well, Mark Miller
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        This switches to using an assert to enable the test code rather than the system property. We don't always necessarily use jetty.testMode and we would like to be sure this is checked for every test. I did have to add a system property to turn off the check for JettyWebappTest though (jetty code closes the stream but it trips our protection assert).

        Show
        markrmiller@gmail.com Mark Miller added a comment - This switches to using an assert to enable the test code rather than the system property. We don't always necessarily use jetty.testMode and we would like to be sure this is checked for every test. I did have to add a system property to turn off the check for JettyWebappTest though (jetty code closes the stream but it trips our protection assert).
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I'm also going to randomly disable the test mode in SolrDispatchFilter so we are sure all tests will also run properly without this code.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I'm also going to randomly disable the test mode in SolrDispatchFilter so we are sure all tests will also run properly without this code.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-8933: Solr should not close container streams.

        Show
        jira-bot ASF subversion and git services added a comment - Commit f3de22377486e88ed12427c3bbd3a89c7c051328 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f3de223 ] SOLR-8933 : Solr should not close container streams.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 95c5d92f109b4aa9cbffcfaf0802be266157c1f5 in lucene-solr's branch refs/heads/branch_6x from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=95c5d92 ]

        SOLR-8933: Solr should not close container streams.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 95c5d92f109b4aa9cbffcfaf0802be266157c1f5 in lucene-solr's branch refs/heads/branch_6x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=95c5d92 ] SOLR-8933 : Solr should not close container streams.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Thanks Mike!

        Show
        markrmiller@gmail.com Mark Miller added a comment - Thanks Mike!
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271
        Hide
        steve_rowe Steve Rowe added a comment -

        Reopening to backport to 6.0.2, 5.6 and 5.5.2.

        Show
        steve_rowe Steve Rowe added a comment - Reopening to backport to 6.0.2, 5.6 and 5.5.2.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2049471579db8775dd3df01fd1386c7f43ed4b0e in lucene-solr's branch refs/heads/branch_5_5 from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2049471 ]

        SOLR-8933: Solr should not close container streams.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2049471579db8775dd3df01fd1386c7f43ed4b0e in lucene-solr's branch refs/heads/branch_5_5 from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2049471 ] SOLR-8933 : Solr should not close container streams.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 9c81c77baec53fd60163a3e1d2a4489e081f2eaa in lucene-solr's branch refs/heads/branch_5x from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c81c77 ]

        SOLR-8933: Solr should not close container streams.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 9c81c77baec53fd60163a3e1d2a4489e081f2eaa in lucene-solr's branch refs/heads/branch_5x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c81c77 ] SOLR-8933 : Solr should not close container streams.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit f6d40e3e668a0c5cd4f11493f11afeaf1a45d267 in lucene-solr's branch refs/heads/branch_5x from Steve Rowe
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f6d40e3 ]

        SOLR-8933: Remove misplaced CHANGES entry

        Show
        jira-bot ASF subversion and git services added a comment - Commit f6d40e3e668a0c5cd4f11493f11afeaf1a45d267 in lucene-solr's branch refs/heads/branch_5x from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f6d40e3 ] SOLR-8933 : Remove misplaced CHANGES entry
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 6f8d306ea8ca96c4c6edfffcfed7c300d328716a in lucene-solr's branch refs/heads/branch_6_0 from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6f8d306 ]

        SOLR-8933: Solr should not close container streams.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 6f8d306ea8ca96c4c6edfffcfed7c300d328716a in lucene-solr's branch refs/heads/branch_6_0 from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6f8d306 ] SOLR-8933 : Solr should not close container streams.
        Hide
        steve_rowe Steve Rowe added a comment -

        Bulk close issues released with 5.5.2.

        Show
        steve_rowe Steve Rowe added a comment - Bulk close issues released with 5.5.2.

          People

          • Assignee:
            markrmiller@gmail.com Mark Miller
            Reporter:
            mdrob Mike Drob
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development