Repository navigation
fix(sheet): isolate column width cache per sheet holder - #1306
yebuguntang wants to merge 3 commits into
Conversation
…et holder Move the per-column maximum width cache from the strategy instance onto WriteSheetHolder so that a single LongestMatchColumnWidthStyleStrategy instance can be reused across writers without leaking widths from one workbook or sheet into another. The strategy is now stateless with respect to width tracking, which also removes the shared mutable state discussed in apache#1305 (thread-safety and reference strength are no longer concerns). This follows the same per-holder isolation pattern used for converter state (e.g. apache#740). fixes apache#1305
Mikkey-f
left a comment
There was a problem hiding this comment.
Thanks @yebuguntang — I reproduced the leak and verified the fix locally (Windows 11, JDK 17).
Root cause: The strategy cache is keyed by sheetNo only, so two ExcelWriters both use key 0. Since a width is only applied when it beats the cached maximum, the second workbook's shorter value never reaches setColumnWidth and stays at POI's default.
Verification
- Red: with only the two product files reverted to current main (your tests kept),
sharedStrategyInstanceDoesNotLeakWidthAcrossWorkbooksfails withexpected: <256> but was: <2048>; the separate-instance control test still passes. - Green: both tests pass (2/2).
- Full suite (
mvn clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet): 60 + 1,791 tests, 0 failures;spotless:checkclean.
Code review: Moving the cache to WriteSheetHolder gives it the right owner — one map per sheet, lifetime tied to the workbook, and it still accumulates the running maximum across doWrite calls on the same sheet. Excluding it from @EqualsAndHashCode matches the existing hasBeenInitializedTable pattern. SimpleColumnWidthStyleStrategy and AbstractHeadColumnWidthStyleStrategy don't hold per-instance state, so there's no sibling copy of this bug.
Two nits (non-blocking)
WriteSheetHolder.columnWidthCachehas no javadoc while the surrounding fields do — a one-liner would keep the holder self-documenting.- The new test class javadoc still says "per-instance width cache"; after the fix it's per sheet holder.
LGTM — correct state ownership and a deterministic regression test.
|
Thank you @Mikkey-f for the careful review and for taking the time to reproduce the leak and verify the fix — it really helped. I also overlooked the project's documentation conventions: the new field should have had a javadoc, and the test comment should have been updated along with the code. Lesson learned — I'll pay closer attention to javadoc and to keeping comments in sync in future contributions. Thanks again for the guidance, much appreciated! |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the change to isolate column-width tracking per sheet holder.
No open issues 🎉
What changed in this PR
Fixes #1305 by moving column-width tracking from the reusable strategy to each sheet holder, preventing cached widths from leaking across workbooks.
Changes:
- Adds a per-holder column-width cache, excluded from equality and hash-code calculations.
- Updates the strategy to lazily initialize and use the holder’s cache.
- Adds real-file tests covering separate strategy instances and one instance shared across writers.
| File | Description |
|---|---|
fesod-sheet/src/test/java/org/apache/fesod/sheet/write/style/column/LongestMatchColumnWidthStyleStrategyTest.java |
Adds control and regression tests that verify saved workbook widths. |
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/style/column/LongestMatchColumnWidthStyleStrategy.java |
Replaces strategy-owned width tracking with the sheet holder’s cache. |
fesod-sheet/src/main/java/org/apache/fesod/sheet/write/metadata/holder/WriteSheetHolder.java |
Adds the isolated width cache and excludes it from equality and hashing. |
Review effort: Balanced
Summary
fixes #1305
LongestMatchColumnWidthStyleStrategycached the max column width in a field keyed only bysheetNo. When the same strategy instance was reused across twoExcelWriterinstances (or two workbooks), both usedsheetNo = 0, so the second workbook's widths could never beat the value cached from the first one. As a resultSheet.setColumnWidthwas never called and the second workbook fell back to POI's default column width.Fix
Move the per-column max-width cache from the strategy instance onto
WriteSheetHolder(columnWidthCache). The strategy is now stateless with respect to width tracking, so:This follows the same per-holder isolation pattern used for converter state (e.g. #740), keeping related state co-located with the holder that owns it.
Test
Added
LongestMatchColumnWidthStyleStrategyTest:separateStrategyInstancesComputeEachWorkbookWidth— control case (separate instances).sharedStrategyInstanceDoesNotLeakWidthAcrossWorkbooks— regression case that fails against the old cache-on-strategy implementation and passes after the fix.Notes
The two design points discussed in #1305 (strong vs. weak references, thread safety) are no longer relevant because the cache is now per-holder and not shared.