[
https://issues.apache.org/jira/browse/SOLR-10778?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16031317#comment-16031317
]
Erick Erickson edited comment on SOLR-10778 at 5/31/17 3:13 PM:
----------------------------------------------------------------
So I worked through the warnings relating to JavaBinCodec last night. Most of
them were in tests but a few others were concerning. My guess is that since new
JavaBinCodec().marshal "did the right thing", lead to code that used the same
pattern with unmarshal, which does not call finish. See
BinaryResponseWriter.getParsedResponse.
Anyway, that leads to several questions:
1> Is this worth pursuing? I strongly favor having the compiler help me all it
can. I've picked on JavaBinCodec since it's the first one I saw, but there are
others.
2> Let's claim we get almost all of our warnings gone . Can we tag allowable
instances of not closing a resource so precommit lets them by but fail any
others? See below.
3> On a quick scan, the overwhelming majority of these are in tests. How much
effort do we want to expend in the tests? And could/should my hypothetical
"fail precommit" bits somehow skip tests?
There's this code in SegmentReader. The line I've tagged with //HERE complains
"'dir' is not closed at this location". And it shouldn't be.
{code}
private DocValuesProducer initDocValuesProducer() throws IOException {
final Directory dir = core.cfsReader != null ? core.cfsReader : si.info.dir;
if (!fieldInfos.hasDocValues()) {
return null; //HERE
} else if (si.hasFieldUpdates()) {
return new SegmentDocValuesProducer(si, dir, core.coreFieldInfos,
fieldInfos, segDocValues);
} else {
// simple case, no DocValues updates
return segDocValues.getDocValuesProducer(-1L, si, dir, fieldInfos);
}
}
{code}
Finally, note that I am _NOT_ advocating implementing complex rewrites just to
make lint happy. OTOH, I _AM_ strongly in favor of taking advantage of all the
automated tools we can. If there's a good reason to not close a resource (the
code snippet above is an example) we need to just be able to tag it. What I'm
hoping for is a way to help me not make mistakes either through carelessness or
lack of understanding. But if we go through the effort of cleaning all this up,
I'd _really_ like a way to not have the problem creep back.
was (Author: erickerickson):
So I worked through the warnings relating to JavaBinCodec last night. Most of
them were in tests but a few others were concerning. My guess is that since new
JavaBinCodec().marshal "did the right thing", lead to code that used the same
pattern with unmarshal, which does not call finish. See
BinaryResponseWriter.getParsedResponse.
Anyway, that leads to several questions:
1> Is this worth pursuing? I strongly favor having the compiler help me all it
can. I've picked on JavaBinCodec since it's the first one I saw, but there are
others.
2> Let's claim we get almost all of our warnings gone . Can we tag allowable
instances of not closing a resource so precommit lets them by but fail any
others? See below.
3> On a quick scan, the overwhelming majority of these are in tests. How much
effort do we want to expend in the tests? And could/should my hypothetical
"fail precommit" bits somehow skip tests?
There's this code in SegmentReader. The line I've tagged with //HERE complains
"'dir' is not closed at this location". And it shouldn't be.
private DocValuesProducer initDocValuesProducer() throws IOException {
final Directory dir = core.cfsReader != null ? core.cfsReader : si.info.dir;
if (!fieldInfos.hasDocValues()) {
return null; //HERE
} else if (si.hasFieldUpdates()) {
return new SegmentDocValuesProducer(si, dir, core.coreFieldInfos,
fieldInfos, segDocValues);
} else {
// simple case, no DocValues updates
return segDocValues.getDocValuesProducer(-1L, si, dir, fieldInfos);
}
}
Finally, note that I am _NOT_ advocating implementing complex rewrites just to
make lint happy. OTOH, I _AM_ strongly in favor of taking advantage of all the
automated tools we can. If there's a good reason to not close a resource (the
code snippet above is an example) we need to just be able to tag it. What I'm
hoping for is a way to help me not make mistakes either through carelessness or
lack of understanding. But if we go through the effort of cleaning all this up,
I'd _really_ like a way to not have the problem creep back.
> Ant precommit task WARNINGS about unclosed resources
> ----------------------------------------------------
>
> Key: SOLR-10778
> URL: https://issues.apache.org/jira/browse/SOLR-10778
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: clients - java
> Affects Versions: 4.6
> Reporter: Andrew Musselman
> Priority: Minor
> Attachments: notclosed.txt
>
>
> During precommit we are seeing lots of warnings about resources that aren't
> being closed, which could pose problems based on chat amongst the team. Log
> snippet for example:
> [mkdir] Created dir:
> /var/folders/5p/6b46rm_94dzc5m8d4v56tds40000gp/T/ecj1165341501
> [ecj-lint] Compiling 419 source files to
> /var/folders/5p/6b46rm_94dzc5m8d4v56tds40000gp/T/ecj1165341501
> [ecj-lint] ----------
> [ecj-lint] 1. WARNING in
> /path/to/lucene-solr/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java
> (at line 920)
> [ecj-lint] new LBHttpSolrClient(httpSolrClientBuilder, httpClient,
> solrServerUrls) :
> [ecj-lint]
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [ecj-lint] Resource leak: '<unassigned Closeable value>' is never closed
> [ecj-lint] ----------
> [ecj-lint] ----------
> [ecj-lint] 2. WARNING in
> /path/to/lucene-solr/solr/solrj/src/java/org/apache/solr/client/solrj/impl/StreamingBinaryResponseParser.java
> (at line 49)
> [ecj-lint] JavaBinCodec codec = new JavaBinCodec() {
> [ecj-lint] ^^^^^
> [ecj-lint] Resource leak: 'codec' is never closed
> [ecj-lint] ----------
> [ecj-lint] ----------
> [ecj-lint] 3. WARNING in
> /path/to/lucene-solr/solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java
> (at line 90)
> [ecj-lint] JavaBinCodec codec = new JavaBinCodec();
> [ecj-lint] ^^^^^
> [ecj-lint] Resource leak: 'codec' is never closed
> [ecj-lint] ----------
> [ecj-lint] 4. WARNING in
> /path/to/lucene-solr/solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java
> (at line 113)
> [ecj-lint] JavaBinCodec codec = new JavaBinCodec() {
> [ecj-lint] ^^^^^
> [ecj-lint] Resource leak: 'codec' is never closed
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
