BaseDirectoryTestCase should use try-with-resources for its Directories [LUCENE-6916]

apache / lucene

Apache Lucene open-source search software

https://lucene.apache.org/

Apache License 2.0

2.64k stars 1.02k forks source link

BaseDirectoryTestCase should use try-with-resources for its Directories [LUCENE-6916] #7974

Open asfimport opened 8 years ago

asfimport commented 8 years ago

I'm playing around with writing a nio2 FileSystem implementation for HDFS that will work with Directory, and it currently leaks threads everywhere because if a BaseDirectoryTestCase test fails it doesn't close its Directory. This obviously won't be a problem if everything passes, but it will probably be a while before that's true and it makes iterative development a bit of a pain.

Migrated from LUCENE-6916 by Alan Woodward (@romseygeek) Attachments: LUCENE-6916.patch

asfimport commented 8 years ago

Alan Woodward (@romseygeek) (migrated from JIRA)

Patch. I've only wrapped Directory usage with try-with-resources for now, but it could be extended to all the input and outputstreams as well.

asfimport commented 8 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I'm not sure we should do this, it makes test cases significantly harder to read, but at what benefit?

We should not be failing on file leaks if the test already fails.

asfimport commented 8 years ago

Alan Woodward (@romseygeek) (migrated from JIRA)

It's FileSystem leaks rather than file leaks for me, but it's a fairly unusual case I know.

It doesn't seem harder to read to me, but I guess that's subjective anyway :-) I can always just copy the test into my own project and make the changes there if it makes things more difficult for everybody else.

asfimport commented 8 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Well, my issue is that:

try-with definitely adds complexity, its like try/catch but with more complexity on top.
its "optimizing for a failure case"
it is not an issue for any directories in lucene.
once its done here, i am worried about a precedent, where every single lucene test must be in try-with.

asfimport commented 8 years ago

Robert Muir (@rmuir) (migrated from JIRA)

And by the way, I am ok with the change if its contained to just this one file: because its purpose is to test directories. But I don't think we should do this across all tests in general, it has a real cost.