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 Sun, 06 May 2018 19:51:00 GMT

    [ 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message