You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2026/09/07 11:49:26 UTC
[PR] Stop MainExtractorFactory swallowing fatal errors [poi]
pjfanning opened a new pull request, #1254:
URL: https://github.com/apache/poi/pull/1254
Both `create(File, String)` and `create(InputStream, String)` guard their cleanup with an inverted fatal check:
```java
} catch (Throwable t) {
if (!ExceptionUtil.isFatal(t)) {
fs.close();
throw t;
}
}
```
For a **fatal** throwable — `VirtualMachineError`, `ThreadDeath` — the catch body does nothing at all. Execution falls through to the `if (extractor == null)` branch and the method returns `null`. So an `OutOfMemoryError` raised while building the extractor is reported to the caller as "no extractor for this file".
The rest of the codebase uses the opposite form, rethrowing fatal throwables rather than suppressing them — `XMLHelper.java:177`, `CleanerUtil.java:184`, `HSSFParser.java:53`.
### Fix
There is nothing here that needs to treat fatal throwables differently: the only work in the catch block is closing a filesystem this method opened and owns. So rather than correcting the condition to `if (ExceptionUtil.isFatal(t)) ExceptionUtil.rethrow(t);`, the check is dropped entirely and every throwable propagates.
`fs.close()` also becomes `IOUtils.closeQuietly(fs)` so a failure while closing cannot mask the original throwable — `ExtractorFactory.createExtractor(File, String)` uses the same closeQuietly-then-rethrow shape.
The filesystem is still closed on every failure path, so this does not reintroduce the leak that the original catch block was there to prevent.
This came out of the same review as #1248 — #1253, but it is a correctness bug rather than a leak, so it is separate.
No public signatures change, so there is nothing for MiMa to check. `TestMainExtractorFactory` and `TestExtractorFactory` (120 tests) pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: [PR] Stop MainExtractorFactory swallowing fatal errors [poi]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #1254:
URL: https://github.com/apache/poi/pull/1254
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org