[ 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