[
https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465276#comment-16465276
]
Uwe Schindler edited comment on SOLR-12290 at 5/6/18 7:50 PM:
--------------------------------------------------------------
Sorry, I am a bit worried today. It's too hot here and it was the yonly day
where I fixed a serious security issue caused by me 5 years ago and was really
annoyed about tests failing all day.
You said "there is no need to close streams if closing is a no-op". I still
argue that this is the wrong way do do it. If stuff like Jetty needs special
handling on closing, it should be done top-level. If downstream code gets a
stream, it should do try-with-resources.
I am happy with your code in SolrDispatchFilter and the wrapper around the
ServletXxxStreams. But I don't think we should make users forcefully remove
try-with-resources blocks (and cause a warning in Eclipse), just because some
specific implementation of a stream needs special handling. So I'd put all
special case only in SolrDispatchFilter and whenever a user gets an input
stream/outputstream further down the code it MUST close it. That's just a fact
of good programming practise. A method gets a stream does something with it and
closes it. Solr (especially in tests) is already full of missing closes, so we
should not add more. And because of that I am heavily arguing. It was not
against you, I was just bored about the sometimes horrible code quality of solr
and its tests and a commit that made the code quality of some parts against all
programming standards (streams have to be closed after usage). One reason that
I try to avoid fixing bugs in Solr, unless they were caused by me or have
something to do with XML handling (because that's one of my beloved parts of
code - I love XML).
I can confirm the tests now pass on Windows. So file leaks with uploaded files
or other types of content streams are solved. Thanks, but I have a bad feeling
now about one more horrible anti-feature of solr.
was (Author: thetaphi):
Sorry, I am a bit worried today. It's too hot here and it was the yonly day
where I fixed a serious security issue caused by me 5 years ago and was really
annoyed about tests failing all day.
You said "there is no need to close streams if closing is a no-op". I still
argue that this is the wrong way do do it. If stuff like Jetty needs special
handling on closing, it should be done top-level. If a user gets a
I am happy with your code in SolrDispatchFilter and the wrapper around the
ServletXxxStreams. But I don't think we should make users forcefully remove
try-with-resources blocks (and cause a warning in Eclipse), just because some
specific implementation of a stream needs special handling. So I'd put all
special case only in SolrDispatchFilter and whenever a user gets an input
stream/outputstream further down the code it MUST close it. That's just a fact
of good programming practise. A method gets a stream does something with it and
closes it. Solr (especially in tests) is already full of missing closes, so we
should not add more. And because of that I am heavily arguing. It was not
against you, I was just bored about the sometimes horrible code quality of solr
and its tests and a commit that made the code quality of some parts against all
programming standards (streams have to be closed after usage). One reason that
I try to avoid fixing bugs in Solr, unless they were caused by me or have
something to do with XML handling (because that's one of my beloved parts of
code - I love XML).
I can confirm the tests now pass on Windows. So file leaks with uploaded files
or other types of content streams are solved. Thanks, but I have a bad feeling
now about one more horrible anti-feature of solr.
> 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: 7.4, 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]
