Lucene40StoredFieldsReader's constructor calls close() instead of IOUtils.closeWhileHandlingException in its finally block [LUCENE-4438]
- Dominant language
- Java
- Stars
- 3.6k
- Forks
- 1.4k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 88
Description
It would be nice to have automated tests for this kind of things (similarly to the check-forbidden-api task).
Here is the discussion I just had with Uwe on IRC:
```
15:32 < jpountz> ThetaPh1: I just saw that Lucene40StoredFieldsReader's constructor calls close in its finally block. I think it is wrong since close might throw an IOE (should be catched), am I
correct? If yes, then is it something we could test with ASM (similarly to the forbidden API checks)?
15:32 <@ThetaPh1> it does not use IOUtils?
15:33 <@ThetaPh1> we cannot check this with asm easily
15:33 <@ThetaPh1> we could only forbid calling Closeable.close() and exclude IOUtils from that
15:34 <@ThetaPh1> so the correct fix is to use IOUtils when clsoing
15:34 <@ThetaPh1> it also checks for null and suppresses exceptions
15:36 < jpountz> ThetaPh1: no, it does no
15:36 <@ThetaPh1> its a bug :-)
15:36 <@ThetaPh1> but not serious
15:36 <@ThetaPh1> for local files it never throws exceptions
15:36 < jpountz> ok
15:37 < jpountz> I think catching calls to Closeable.close would already be nice
15:37 <@ThetaPh1> but we might think about disallowing Closeable.close()
15:37 < jpountz> I'll open an issue
15:37 <@ThetaPh1> we can add that as a separate check-forbidden-apis file
15:37 <@ThetaPh1> but exclude IOUtils from the fileset
15:37 <@ThetaPh1> unfortunately all tests do this
15:38 <@ThetaPh1> so i would restriuct this to non-tests, too
```
---
Migrated from [LUCENE-4438](https://issues.apache.org/jira/browse/LUCENE-4438) by Adrien Grand (@jpountz), updated May 09 2016
Contributor guide
Research direction
Start with the Lucene40StoredFieldsReader constructor and inspect its finally block, then read the existing check-forbidden-api task and related check-forbidden-apis configuration. Compare the current close() call with IOUtils.closeWhileHandlingException and review how non-test files are selected. Done means the constructor handling is corrected and an automated check or test covers this pattern without flagging IOUtils or tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100