fix: keep the first failure and suppress later ones

[nkuprins]

Closed: #1136

Purpose of the pull request

As title

What's changed?

• WriteContextImpl#finish keeps the first failure as the cause and every later failure is attached to it with addSuppressed, so the full chain is kept.
• New WriteContextImplTest write and close both fail. It checks that the write failure is the cause and that the close failure is its only suppressed exception.

Checklist

• I have read the Contributor Guide.
• I have written the necessary doc or comment.
• I have added the necessary unit tests and all cases have passed.

[bengbengbalabalabeng]

Currently, only the first exception is preserved. If multiple cleanup steps fail, all subsequent exceptions are silently swallowed, which can make troubleshooting in production quite difficult.

Could we consider using Throwable#addSuppressed to preserve the complete exception chain:

private static Throwable recordFailure(Throwable recorded, Throwable t) {
    if (recorded == null) {
        return t;
    }
    if (t != null && recorded != t) {
        recorded.addSuppressed(t);
    }
    return recorded;
}

[nkuprins]

Currently, only the first exception is preserved. If multiple cleanup steps fail, all subsequent exceptions are silently swallowed, which can make troubleshooting in production quite difficult.

Could we consider using Throwable#addSuppressed to preserve the complete exception chain:

private static Throwable recordFailure(Throwable recorded, Throwable t) {
    if (recorded == null) {
        return t;
    }
    if (t != null && recorded != t) {
        recorded.addSuppressed(t);
    }
    return recorded;
}

Yes, this was the alternative approach in the issue. One of the reasons I kept it to the first failure: before alibaba/easyexcel@2c8918be, each of these catch blocks threw immediately, so the first failure was always the one reported.

[bengbengbalabalabeng]

LGTM.

[BigDataDZ]

LGTM - the mechanism is sound and the test proves the exact scenario from #1136 (write failure as cause, close failure as its only suppressed). Nice detail guarding recorded != t, which prevents the IllegalArgumentException self-suppression case.

Two notes:

1. The same last-wins pattern exists on the read side. ExcelAnalyserImpl's cleanup sequence has six identical throwable = t; overwrites (read cache destroy, OPCPackage#revert, PoifsFileSystem#close, CSV parser close, stream close, temp file delete, around lines 230-282 on current main). A read whose workbook closes fine but whose temp file delete then fails (the inverse of #1136's write case) reports the delete error and swallows the close error today. Worth a follow-up issue/PR applying the same first-failure-plus-suppressed shape there - happy to take that, or leave it to you.

2. Nit, non-blocking: the t != null check is defensive dead code today since every call site catches Throwable (never null) - harmless to keep.

[BigDataDZ]

Following up on my note above: filed the read-side counterpart as #1204 with a fix up in #1205 (same first-failure-plus-suppressed shape, regression test forcing two cleanup failures). Feel free to review it there since you wrote the original shape.