[
https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465479#comment-16465479
]
Uwe Schindler commented on SOLR-12290:
--------------------------------------
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: [email protected]
For additional commands, e-mail: [email protected]
