lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Uwe Schindler (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
Date Mon, 07 May 2018 06:25:00 GMT

    [ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465479#comment-16465479
] 

Uwe Schindler edited comment on SOLR-12290 at 5/7/18 6:24 AM:
--------------------------------------------------------------

Hi,
I can backport this if needed. I'd just like to have a second iteration on it. I am not really
happy with the stuff (especially when handling gzip responses). 

I understand the issues here, I just wanted to have an easier way to consume the Solr Request
where everything is handled in SolrRequestParsers and SolrDispatchFilter. The downstream code
should just not need to thing about ("do I need to close or not?). The code should always
use try-with-resources and any stuff that was not yet read from the underlying servlet stream
should be consumed before SolrDispatchFilter gives control back to the Jetty container. I
am out of office this week, so I will for sure look into it next weekend.

bq. We are still still not closing servlet streams where we don't have to. In some cases we
have because the user of the API can't discern where the stream came from. This wasn't a coding
practice issue, it was a bug. -- Being an impatient jerk is an unnecessary part of the process.

Sorry for being impatient jerk! It was also missing communication. I asked for a revert initially,
because the issue was a file handle leak. This was my first comment. It was just a normal
request, not even a veto: I was asking for revert so we can have a look a second time. I was
not really following this issue last week, because we were moving offices and so on, maybe
I should have looked earlier. 

I then looked into your patch and was understanding and seeing the issue. I was totally fine
with keeping the ServletInputStream open, but the bug in the ContentHandler code looked like
bad coding practise, because it removed necessary try-with-resources. I brought that up on
the issue, but the reaction was - as far as I understand - it something like: "no we don't
need to close streams so I refuse to add CloseShield". This was the misunderstanding (you
talked about the servlet stream, I talked about ContentStreams). And then I proposed to add
a patch. While doing this you committed almost the same code that I hacked together (I reverted
about 3 or 4 files and added the CloseShield). The "This was my patch" was just a confirmation:
All fine! Sorry if you have thought

In some cases I am a bit like Robert, especially if it is around closing stuff and file leaks.
I try to write code in a way that the downstream code is forced to always write "correct code"
(means closing everything with try-with-resources) and we should work around bugs like Jetty's
at the source of the problem without touching any other code. IMHO, the asserts in test code
are not really needed. Just shield the ServletInputStream from being closed in SolrDispatchFilter
and if the downstream code calls close and it is not yet fully consumed, just copy the remaining
content to Java's "/dev/null" with IOUtils.


was (Author: thetaphi):
Hi,
I can backport this if needed. I'd just like to have a second iteration on it. I am not really
happy with the stuff (especially when handling gzip responses). 

I understand the issues here, I just wanted to have an easier way to consume the Solr Request
where everything is handled in SolrRequestParsers and SolrDispatchFilter. The downstream code
should just not need to thing about ("do I need to close or not?). The code should always
use try-with-resources and any stuff that was not yet read from the underlying servlet stream
should be consumed before SolrDispatchFilter gives control back to the Jetty container. I
am out of office this week, so I will for sure look into it next weekend.

bq. We are still still not closing servlet streams where we don't have to. In some cases we
have because the user of the API can't discern where the stream came from. This wasn't a coding
practice issue, it was a bug. -- Being an impatient jerk is an unnecessary part of the process.

Sorry for being impatient jerk! It was also missing communication. I asked for a revert initially,
because the issue was a file handle leak. This was my first comment. It was just a normal
request, not even a veto: I was asking for revert so we can have a look a second time. I was
not really following this issue last week, because we were moving offices and so on, maybe
I should have looked earlier. 

I then looked into your patch and was understanding and seeing the issue. I was totally fine
with keeping the ServletInputStream open, but the bug in the ContentHandler code looked like
bad coding practise, because it removed necessary try-with-resources. I brought that up on
the issue, but the reaction was - as far as I understand it something like - "no we don't
need to close streams so I refuse to add CloseShield". This was the misunderstanding (you
talked about the servlet stream, I talked about ContentStreams". And then I proposed to add
a patch. While doing this you committed almost the same code that I hacked together (I reverted
about 3 or 4 files and added the CloseShield). The "This was my patch" was just a confirmation:
All fine! Sorry if you have thought

In some cases I am a bit like Robert, especially if it is around closing stuff and file leaks.
I try to write code in a way that the downstream code is forced to always write "correct code"
(means closing everything with try-with-resources) and work around bugs like Jettys at the
source of the problem without touching any other code. IMHO, the asserts in test code are
not really needed. Just shield the ServletInputStream from being closed in SolrDispatchFilter
and if the downstream code calls close and it is not yet fully consumed, just copy it to Java's
"/dev/null" with IOUtils.

> Do not close any servlet streams and improve our servlet stream closing prevention code
for users and devs.
> -----------------------------------------------------------------------------------------------------------
>
>                 Key: SOLR-12290
>                 URL: https://issues.apache.org/jira/browse/SOLR-12290
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>            Reporter: Mark Miller
>            Assignee: Mark Miller
>            Priority: Major
>             Fix For: master (8.0)
>
>         Attachments: SOLR-12290.patch, SOLR-12290.patch, SOLR-12290.patch, SOLR-12290.patch
>
>
> Original Summary:
> When you fetch a file for replication we close the request output stream after writing
the file which ruins the connection for reuse.
> We can't close response output streams, we need to reuse these connections. If we do
close them, clients are hit with connection problems when they try and reuse the connection
from their pool.
> New Summary:
> At some point the above was addressed during refactoring. We should remove these neutered
closes and review our close shield code.
> If you are here to track down why this is done:
> Connection reuse requires that we read all streams and do not close them - instead the
container itself must manage request and response streams. If we allow them to be closed,
not only do we lose some connection reuse, but we can cause spurious client errors that can
cause expensive recoveries for no reason. The spec allows us to count on the container to
manage streams. It's our job simply to not close them and to always read them fully, from
client and server. 
> Java itself can help with always reading the streams fully up to some small default amount
of unread stream slack, but that is very dangerous to count on, so we always manually eat
up anything on the streams our normal logic ends up not reading for whatever reason.
> We also cannot call abort without ruining the connection or sendError. These should be
options of very last resort (requiring a blood sacrifice) or when shutting down.
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message