[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16498892#comment-16498892 ] ASF subversion and git services commented on SOLR-12290: Commit 88400a14716f163715eac82a35be90e6e3718208 in lucene-solr's branch refs/heads/branch_7x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=88400a1 ] SOLR-12290,SOLR-12391: Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs. > 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, SOLR-12290_7x.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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16498886#comment-16498886 ] ASF subversion and git services commented on SOLR-12290: Commit 1ff24bbb283a4cbfb9a6babfa702fbd57804a7fd in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1ff24bb ] SOLR-12290,SOLR-12391: Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs. > 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, SOLR-12290_7x.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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16494006#comment-16494006 ] Varun Thacker commented on SOLR-12290: -- Hi Mark, I committed it and looks like the author tag was also retained . So the commit technically looks like it's come from you . In master , the CHANGES entry is already under 7.4 so nothing to do there. > 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, SOLR-12290_7x.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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16493988#comment-16493988 ] ASF subversion and git services commented on SOLR-12290: Commit 62de5c099476c958229aafe19c3b266fdfec10eb in lucene-solr's branch refs/heads/branch_7x from [~mark.mil...@oblivion.ch] [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=62de5c0 ] SOLR-12290: Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs. (cherry picked commit 2962010 + 5fc7251 + 4c09a13 + ab58b7f) > 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, SOLR-12290_7x.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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16492978#comment-16492978 ] Varun Thacker commented on SOLR-12290: -- Sounds good! Here's a patch which squashes all the 4 commits into one. I'll run tests and precommit before pushing this > 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, SOLR-12290_7x.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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16492948#comment-16492948 ] Mark Miller commented on SOLR-12290: Yeah, feel free - the reason I’m not very concerned is that the other issue I did around around the same time that gives updates and only updates their own connection pool should remove any issue this would prob help with. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16492883#comment-16492883 ] Varun Thacker commented on SOLR-12290: -- Hi Mark, Is it okay if we backport this to branch_7x ? The motivation being that you mentioned on SOLR-1881 ( https://issues.apache.org/jira/browse/SOLR-11881?focusedCommentId=16458322&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16458322 ) that this could be one of the underlying reasons for the Jetty exception. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16472912#comment-16472912 ] ASF subversion and git services commented on SOLR-12290: Commit ab58b7f9ba646a294005f1e433e4faee43c7d22b in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ab58b7f ] SOLR-12290: Update assert messages about closing request / response streams. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16472902#comment-16472902 ] ASF subversion and git services commented on SOLR-12290: Commit 4c09a13afb441bdce1c440263ea61cdb5f10b141 in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4c09a13 ] SOLR-12290: Update assert messages about closing request / response streams. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[
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 t
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465335#comment-16465335 ] Mark Miller commented on SOLR-12290: bq. See my comment. But some things have to be said. Introducing bad programming practises, just because of a special case or bug in some component (Jetty) is not a good idea. I stop talking here, all is said. We didn't introduce bad coding practices and your opinion on how Jetty handles this has little to do with it. The original patch didn't handle ContentStream stuff right when working around assert errors. You are supposed to point out bugs. Being an impatient jerk is an unnecessary part of the process. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465334#comment-16465334 ] Mark Miller commented on SOLR-12290: This wasn't about coding practices - it's simply about the ContentStream API. It can return a stream from any source and so streams that come from that API must be closed. 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. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465331#comment-16465331 ] David Smiley commented on SOLR-12290: - Last commit to partially revert looks good to me. I agree with Uwe's sentiment about standard coding practices – if you obtain the stream, close the stream. I should have thought of this earlier in my review. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465280#comment-16465280 ] Uwe Schindler commented on SOLR-12290: -- See my comment. But some things have to be said. Introducing bad programming practises, just because of a special case or bug in some component (Jetty) is not a good idea. I stop talking here, all is said. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465277#comment-16465277 ] Mark Miller commented on SOLR-12290: bq. Oh thanks, that was my patch Your welcome. If you insist on behaving like Robert, I'm leaving Solr like I left Lucene. There is no reason for this garbage. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465276#comment-16465276 ] Uwe Schindler commented on SOLR-12290: -- 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465271#comment-16465271 ] Mark Miller commented on SOLR-12290: You definitely bring a nasty Lucene vibe into the Solr community sometimes. When did I say I wasn't willing to fix it? I said the fix was easy and I have to run tests. Cool down man. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465269#comment-16465269 ] Uwe Schindler commented on SOLR-12290: -- Oh thanks, that was my patch :-) > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465267#comment-16465267 ] ASF subversion and git services commented on SOLR-12290: Commit 5fc725154001c6283315802e1a2193d51d00f9aa in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5fc7251 ] SOLR-12290: We must close ContentStreams because we don't know the source of the inputstream - use a CloseShield to prevent tripping our close assert in SolrDispatchFilter. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465255#comment-16465255 ] Uwe Schindler commented on SOLR-12290: -- As you are not willing to fix this, should I send you the patch based on your current commit? > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465254#comment-16465254 ] Uwe Schindler commented on SOLR-12290: -- bq. Whether Jetty closed the socket or not, we can't have anyone close these stream because we have to make sure they are fully consumed. I agree that's fine as workaround for the "jetty bug" (it might be one or now, I don't want to argue about it - I'm not Robert). I can just say: Tomcat fully consumes the stream automatically if a consumer closes it. My complaint was only: If we have a perfect stream handling inside SolrDispatchFilter that handles the stream consuming and prevents closing of underlying ServletOutputStream, the code down the line can handle the input like a normal stream. And so goes for ContentStreams, which are an abstract interface. Here I will behave like "Robert" and cry for revert if you do not revert the broken code! > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465252#comment-16465252 ] Uwe Schindler commented on SOLR-12290: -- It's not only CSV handler. It's all code that consumes ContentStream. It's not only tests. E.g. if you upload a file to DIH or you upload the CSV file via HTTP file upload (and not as part of the stream), you get a file stream. So please, just close the stream, costs nothing if its a no-op. It has nothing to do with tests vs. production! In case of a ContentStream, YOU DO NOT KNOW WHAT TYPE OF INPUT STREAM YOU HAVE BEHIND THE ABSTRACT INTERFACE. It can be anything, so it has to be closed. In the case of HttpRequestContentStream it's a no-op, but consuming code does not need to know this. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465249#comment-16465249 ] Mark Miller commented on SOLR-12290: Because we no longer have two separate code paths for tests and normal runs, we have an assert that lets developers know when their closes are no ops so that developers can understand what is actually going on. We don't need a close when it won't actually do anything. We should just close where we need to. In cases like the CSVHandler we need to. Whether Jetty closed the socket or not, we can't have anyone close these stream because we have to make sure they are fully consumed. Anyway, it's simple to fix, I'm not going to jam anything in though, I have to run tests and what not. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465248#comment-16465248 ] Uwe Schindler commented on SOLR-12290: -- I think your patch is fine. Really just revert the code that prevents closing of ContentStream.getStream() stuff. That makes half of the patch obsolete! So it looks like you are doing the wrapping AND preventing closing in solr stuff that reads from those ContentStreams? Why do both if the first already solves the issue? > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465244#comment-16465244 ] Uwe Schindler commented on SOLR-12290: -- I have seen. It's fine! I was just really annoyed today while running tests locally. All tests that use ContentStreams with file uploads or files break horribly on windows (which is a sign for file dexcriptor leaks, so I am glad that we run tests on Windows). As said before: I'd just revert the code where we consume the ContentStream subclasses (BlobHandler, CSVHandler, maybe DIH,...) and just add another CloseShield in the HttpRequestContentStream. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465241#comment-16465241 ] Mark Miller commented on SOLR-12290: I will look into this Uwe. We are still using a CloseShield. Relax, this is only on master and is not a very large change from what we were doing before. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465240#comment-16465240 ] Uwe Schindler commented on SOLR-12290: -- I think I know how to solve this: - Revert all stuf that removed close of code that uses ContentStream - In SolrRequestParsers$HttpRequestContentStream add a CloseShieldInputStream in the getStream() method. This should solve the file leaks caused by no longer closing streams on files... This should make the changes in BlobHandler and CSVLoaderBase obsolete - and code clean again. It's a no-go to not close streams of unknown origin! > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465237#comment-16465237 ] Uwe Schindler commented on SOLR-12290: -- I just repeat: We have no a serious file descriptor leak in combination with ContentStreams!!! > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16465235#comment-16465235 ] Uwe Schindler commented on SOLR-12290: -- This commit breaks TestCSVLoader on Windows. It looks like Solr no longer closes any content streams that are passed separately to the servlet stream: [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestCSVLoader -Dtests.method=testLiteral -Dtests.seed=B40869AE03F63CBC -Dtests.locale=ms -Dtests.timezone=ECT -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 0.06s | TestCSVLoader.testLiteral <<< [junit4]> Throwable #1: java.nio.file.FileSystemException: C:\Users\Uwe Schindler\Projects\lucene\trunk-lusolr1\solr\build\solr-core\test\J0\temp\solr.handler.TestCSVLoader_B40869AE03F63CBC-001\TestCSVLoader-006\solr_tmp.csv: Der Prozess kann nicht auf die Datei zugreifen, da sie von einem anderen Prozess verwendet wird. [junit4]>at __randomizedtesting.SeedInfo.seed([B40869AE03F63CBC:B2ACAF677D87833E]:0) [junit4]>at sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:86) [junit4]>at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:97) [junit4]>at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:102) [junit4]>at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:269) [junit4]>at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103) [junit4]>at java.nio.file.Files.delete(Files.java:1126) [junit4]>at org.apache.solr.handler.TestCSVLoader.tearDown(TestCSVLoader.java:64) [junit4]>at java.lang.Thread.run(Thread.java:748) I am not sure behind the reasoning of forcefully preventing closing of ServletInputStreams - but IMHO, the better way to handle this is (we had this discussion already a while ago): Wrap the ServletInput/ServletOutput streams with a CloseShieldXxxxStream and only pass this one around. This allows that code anywhere in solr is free to close any streams correctly (which it should), but the ServletStreams are kept open by the shield. The issue we have no is that ContentStreams are not necessarily (like in BlobUploadHandler, CSVHandler) coming from the servlet streams. If it is a file or a uploaded file, then we HAVE to close the stream. The reason behind everything here is a bug in Jetty - in contrast to Tomcat and all other servlet containers, it closes the socket after you close the servlet streams. This is a bug - sorry! Jetty should prevent closing the stream! Please revert the current commit. I can help in solving this correctly - unfortunately I am on travel next week. > 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
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16464484#comment-16464484 ] ASF subversion and git services commented on SOLR-12290: Commit 296201055f24f01e1610f2fb87aba7fa90b9dda1 in lucene-solr's branch refs/heads/master from [~mark.mil...@oblivion.ch] [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2962010 ] SOLR-12290: Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16460143#comment-16460143 ] Mark Miller commented on SOLR-12290: For a long term strategy to improve all of this yet again (of course all the previous HttpClient Http1.1 connection failure work was always a stop gap band aid and as we found out extremely hard to fully nail down), I filed SOLR-12297. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16458676#comment-16458676 ] Mark Miller commented on SOLR-12290: As I've been working on this, I realized that we don't ensure reading full streams when we get files for replication, whether we close the streams early or not. I filed SOLR-12293. We need to isolate our update connection pool from these other random uses. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16458594#comment-16458594 ] Mark Miller commented on SOLR-12290: Yeah, it’s not super obvious, but the problem with closing ourselves is that the we can’t clear any data that might be left on the stream and we also can’t gaurantee some data does t show up from the other side right after an early close. So to ensure connection reuse you cannot close yourself. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16458566#comment-16458566 ] David Smiley commented on SOLR-12290: - Sounds good Mark! Thanks for the clarifications. I didn't know this problem could lead to client recovery; ouch! > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-12290) Do not close any servlet streams and improve our servlet stream closing prevention code for users and devs.
[ https://issues.apache.org/jira/browse/SOLR-12290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16458314#comment-16458314 ] Mark Miller commented on SOLR-12290: bq. JavabinLoader: you can now inline parseAndLoadDocs Inlined. I'll commit this in the next couple days. It extends test and safety coverage to the entire SolrDispatchFilter for the first time as well as to our Servlets. It also removes the separate test code path, and so testing and protecting no longer have different coverage and everything is tested and protected on one code path. I've also returned this to a bug. Given SOLR-11692 and some places we never protected (the other servlets) and the fact that different things have been covered at different points of time, this will solve existing spurious cluster recovery issues. > 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
