You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "GOODBOY008 (via GitHub)" <gi...@apache.org> on 2026/10/19 10:49:45 UTC
[PR] refactor: narrow catch(Throwable) to catch(Exception) [fesod]
GOODBOY008 opened a new pull request, #1239:
URL: https://github.com/apache/fesod/pull/1239
## Problem
19 sites in `fesod-sheet` catch `Throwable` in resource-cleanup paths and wrap whatever they get into `ExcelGenerateException` / `ExcelAnalysisException`:
```java
} catch (Throwable e) {
finishOnException();
throw new ExcelGenerateException(e);
}
```
Catching `Throwable` means an `OutOfMemoryError` or `StackOverflowError` reaches the caller as an ordinary business exception. The stack trace is preserved as a cause, but the caller sees `ExcelGenerateException` and the real failure mode is buried. It also makes the catch blocks invisible to any `IllegalCatch`-style static check.
## Change
Narrow all 19 to `catch (Exception ...)` so `Error` propagates:
| File | Sites |
|---|---|
| `analysis/ExcelAnalyserImpl.java` | 8 |
| `context/WriteContextImpl.java` | 6 |
| `write/ExcelBuilderImpl.java` | 3 |
| `ExcelWriter.java` | 1 |
| `ExcelReader.java` | 1 |
| **Total** | **19** |
`5 files changed, 19 insertions(+), 19 deletions(-)` — the replacement is one token per site, nothing else moved.
**Two groups are deliberately left alone:**
- `XlsxSaxAnalyser.java:285,289,293` — these three are part of the XXE fail-closed work and need a different replacement (`SAXException | ParserConfigurationException` plus fallback features), so they go in a separate change.
- `fuzz/ExcelReadFuzzTest.java:49` and `fuzz/XlsReadFuzzTest.java:53` — the fuzz targets walk the cause chain of any `Throwable`, including `Error`, to tell benign parse failures from real findings. Narrowing to `Exception` would let a `StackOverflowError` skip that check and change the fuzz semantics. Those stay as-is and get a checkstyle suppression instead.
## Verification
Full build with tests on the three affected modules:
```console
$ ./mvnw -B package -pl fesod-common,fesod-shaded,fesod-sheet
[INFO] Tests run: 1118, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
```
`catch (Throwable` count in `fesod-sheet/src/main/java` drops from 22 to 3, and the 3 remaining are exactly the `XlsxSaxAnalyser` ones reserved for the XXE change:
```console
$ grep -rn "catch (Throwable" fesod-sheet/src/main/java
analysis/v07/XlsxSaxAnalyser.java:285
analysis/v07/XlsxSaxAnalyser.java:289
analysis/v07/XlsxSaxAnalyser.java:293
```
## Risk
Behaviour change, intended: an `Error` raised during cleanup now propagates instead of being wrapped. In `WriteContextImpl` that means the remaining `close()` calls in the cleanup chain are skipped once an `Error` is thrown. Given the alternative is reporting an OOM as `ExcelAnalysisException`, propagating is the better failure mode — but it is a visible change and worth a line in the release notes.
No public API change: the methods and their declared exceptions are untouched.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: narrow catch(Throwable) to catch(Exception) [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on code in PR #1239:
URL: https://github.com/apache/fesod/pull/1239#discussion_r4322965844
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java:
##########
@@ -540,7 +540,7 @@ public void finish(boolean onException) {
if (writeExcel) {
try {
isOutputStreamEncrypt = doOutputStreamEncrypt07();
- } catch (Throwable t) {
+ } catch (Exception t) {
Review Comment:
skytin1004 is making a good point here. I agree with that concern.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: narrow catch(Throwable) to catch(Exception) [fesod]
Posted by "GOODBOY008 (via GitHub)" <gi...@apache.org>.
GOODBOY008 commented on code in PR #1239:
URL: https://github.com/apache/fesod/pull/1239#discussion_r4323717050
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java:
##########
@@ -540,7 +540,7 @@ public void finish(boolean onException) {
if (writeExcel) {
try {
isOutputStreamEncrypt = doOutputStreamEncrypt07();
- } catch (Throwable t) {
+ } catch (Exception t) {
Review Comment:
@skytin1004 @nkuprins That makes sense. Let me walk through the flow.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: narrow catch(Throwable) to catch(Exception) [fesod]
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #1239:
URL: https://github.com/apache/fesod/pull/1239#discussion_r4322575230
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/ExcelBuilderImpl.java:
##########
@@ -60,7 +60,7 @@ public ExcelBuilderImpl(WriteWorkbook writeWorkbook) {
} catch (RuntimeException e) {
finishOnException();
throw e;
- } catch (Throwable e) {
+ } catch (Exception e) {
Review Comment:
I walked through what happens when an `Error` comes out of this path. It now escapes without being wrapped, which makes sense, but `finishOnException()` is never called. That means the original error is preserved at the cost of skipping best-effort cleanup.
I think this path should still run cleanup before rethrowing the same `Error`, with any cleanup failure attached as suppressed. Does that direction make sense?
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java:
##########
@@ -540,7 +540,7 @@ public void finish(boolean onException) {
if (writeExcel) {
try {
isOutputStreamEncrypt = doOutputStreamEncrypt07();
- } catch (Throwable t) {
+ } catch (Exception t) {
Review Comment:
The same issue shows up inside the cleanup sequence. If this step throws an `Error`, execution leaves `finish()` immediately and the later workbook close, SXSSF dispose, stream close, temporary-resource cleanup, and ThreadLocal removal never run.
I don't think changing the catch type one site at a time is enough here. Cleanup and propagation probably need to be separated: keep collecting failures while every cleanup step runs, then rethrow the first `Error` unchanged at the end. What do you think?
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] refactor: narrow catch(Throwable) to catch(Exception) [fesod]
Posted by "GOODBOY008 (via GitHub)" <gi...@apache.org>.
GOODBOY008 commented on code in PR #1239:
URL: https://github.com/apache/fesod/pull/1239#discussion_r4367074795
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/ExcelBuilderImpl.java:
##########
@@ -60,7 +60,7 @@ public ExcelBuilderImpl(WriteWorkbook writeWorkbook) {
} catch (RuntimeException e) {
finishOnException();
throw e;
- } catch (Throwable e) {
+ } catch (Exception e) {
Review Comment:
Agreed — head 166e1e3b implements exactly that direction. The three entry points (constructor / `addContent` / `fill`) now funnel through a shared helper:
```java
private RuntimeException finishOnExceptionAndPropagate(Throwable e) {
try {
finishOnException();
} catch (Throwable cleanupFailure) {
ThrowableUtils.addSuppressedQuietly(e, cleanupFailure);
}
if (e instanceof RuntimeException) {
throw (RuntimeException) e;
}
if (e instanceof Error) {
throw (Error) e;
}
return new ExcelGenerateException(e);
}
```
So cleanup always runs before rethrowing, a cleanup failure is attached to the original as suppressed (`addSuppressedQuietly` wraps `addSuppressed` in `catch (Throwable ignore)`, mirroring how `Throwable.addSuppressed` itself treats the exceptional case), and `Error` / `RuntimeException` are rethrown unchanged — only checked exceptions get wrapped. The read side mirrors this with `finishAndPropagate` in `ExcelAnalyserImpl`.
This also fixes a pre-existing masking bug: `finishOnException()` throwing `"Can not close IO."` used to *replace* the original exception entirely; now the original stays primary and the cleanup failure rides along as suppressed.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java:
##########
@@ -540,7 +540,7 @@ public void finish(boolean onException) {
if (writeExcel) {
try {
isOutputStreamEncrypt = doOutputStreamEncrypt07();
- } catch (Throwable t) {
+ } catch (Exception t) {
Review Comment:
Agreed — the rework follows exactly that "separate cleanup from propagation" structure. Inside `WriteContextImpl.finish()` (and `ExcelAnalyserImpl.finish()` on the read side) every cleanup step is now individually guarded and recorded:
```java
} catch (Throwable t) {
throwable = ThrowableUtils.recordFailure(throwable, t);
}
```
First failure stays primary, later ones attach as suppressed — the same semantics the write side already got in #1198 — so workbook close, SXSSF dispose, stream close, temp-file delete and ThreadLocal removal all run even when an earlier step throws. The read side's old last-wins `throwable = t` assignments are unified onto `recordFailure` as well. Each chain then ends with:
```java
if (throwable instanceof Error) {
throw (Error) throwable;
}
```
rethrowing the first `Error` unchanged; everything else is wrapped once as before. The entry points (previous comment) keep the cleanup-then-rethrow shape too, so an `Error` from the main path no longer skips `finishOnException()` / `finish()` either.
A few deliberate scope boundaries, happy to discuss any of them:
- The user dispose-handler callbacks at the top of `WriteContextImpl.finish()` (`afterSheetDispose` / `afterWorkbookDispose`) are still unguarded — a handler `RuntimeException` there still aborts the rest of the chain. Guarding them would change how a handler failure propagates (bare vs wrapped) and would let the output file be completed after a user handler has failed; that felt like a separate behaviour decision rather than cleanup mechanics.
- `clearEncrypt03()` / `removeThreadLocalCache()` cannot throw, so no guard was added.
- `finalize()` in `ExcelWriter` / `ExcelReader` keeps `catch (Throwable)` + log — an `Error` during finalization must not kill the finalizer thread.
- The three `catch (Throwable ignore)` blocks in `XlsxSaxAnalyser` (XXE feature settings) and the two fuzz-test cause-chain walks stay untouched, as in the original version of this PR.
Verified on this head: fesod-common 66 tests and fesod-sheet 1124 tests green, including new tests that inject an `Error` from a `RowWriteHandler` / read listener and assert the stream still gets closed and the original `Error` arrives as the primary with cleanup failures suppressed. The new pipeline tests were reverse-verified — they fail without the corresponding main-code change.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org