Skip to content

[Bug] LongestMatchColumnWidthStyleStrategy leaks widths across workbooks #1305

Open

Description

@yebuguntang

Search before asking

  • I searched the issues and found nothing similar.

Fesod version

main@126dde9 (dev-SNAPSHOT)

JDK version

21.0.10 (Microsoft) — project targets Java 8 (CI matrix: 8/11/17/21/25)

Operating system

Windows 10

Steps To Reproduce

Reuse a single LongestMatchColumnWidthStyleStrategy instance for two writers:

import java.io.File;
import java.util.Collections;
import org.apache.fesod.sheet.FesodSheet;
import org.apache.fesod.sheet.write.style.column.LongestMatchColumnWidthStyleStrategy;
import org.apache.poi.ss.usermodel.Sheet;
import org.apache.poi.ss.usermodel.Workbook;
import org.apache.poi.ss.usermodel.WorkbookFactory;

public class ColumnWidthReuseRepro {
    public static void main(String[] args) throws Exception {
        LongestMatchColumnWidthStyleStrategy strategy = new LongestMatchColumnWidthStyleStrategy();

        File first = new File("first.xlsx");
        File second = new File("second.xlsx");

        FesodSheet.write(first).registerWriteHandler(strategy).sheet()
            .doWrite(Collections.singletonList(Collections.singletonList("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"))); // 30 chars
        FesodSheet.write(second).registerWriteHandler(strategy).sheet()
            .doWrite(Collections.singletonList(Collections.singletonList("A"))); // 1 char

        System.out.println("first  col0 width = " + columnWidth(first));
        System.out.println("second col0 width = " + columnWidth(second));
    }

    private static int columnWidth(File file) throws Exception {
        try (Workbook wb = WorkbookFactory.create(file)) {
            Sheet sheet = wb.getSheetAt(0);
            return sheet.getColumnWidth(0);
        }
    }
}

Current Behavior

first  col0 width = 7680
second col0 width = 2048

The second file keeps POI's default column width (2048 ≈ 8 characters) instead of the expected 256 (1 character). When each writer gets its own strategy instance, the second file correctly comes out as 256 — so the discrepancy is entirely caused by reusing the instance.

Root cause in the code:

  • The cache is keyed only by writeSheetHolder.getSheetNo().
  • A sheet number is unique within one workbook, but two separate writers both use sheet number 0.
  • A column width is only applied when it is larger than the cached maximum, so the second file's 1 never beats the first file's cached 30 and setColumnWidth is never called.
  • Nothing clears the cache when a workbook finishes.

Expected Behavior

Reusing one strategy instance across writers should not carry column widths from a previous workbook into the next. The second file above should end up with a width of 256.

If instance reuse is intentionally unsupported, that limitation should be documented explicitly — silently producing wrong widths is not acceptable.

Proposed Fix

Move the per-sheet maximum column width map onto the WriteSheetHolder instead of keeping it on the strategy instance:

  • Add a columnWidthCache field (Map<Integer, Integer>, keyed by column index) to WriteSheetHolder. The strategy reads/writes it via the holder in setColumnWidth and lazily initializes it.
  • The strategy becomes stateless: it holds no cache, implements no WorkbookWriteHandler/SheetWriteHandler, and needs no dispose hooks.

Why this fixes the bug: each sheet (and therefore each workbook) gets its own holder with its own columnWidthCache. Reusing a single strategy instance across writers can no longer carry widths from one workbook/sheet into another, because there is no shared state left on the strategy. The cache also dies with the holder, so there is no leak even for an abandoned writer.

This is the column-width counterpart of the converter-isolation work in #740, which solved the same "state shared across reused writer instances" class of problem for converters by giving each holder its own copy.

Regression test

A test comparing separate instances (control) with a shared instance reproduces the problem deterministically; I will include it in the PR.

Related

Are you willing to submit a PR?

  • I'm willing to submit a PR!

Activity

  1. yebuguntang commented on Nov 2, 2026

    @yebuguntang
    ContributorAuthor

    Implemented in #1306.
    The PR adopts the Alternative considered in the original report (storing the per-sheet max-width map on WriteSheetHolder) rather than the default 3-step plan (workbook-identity IdentityHashMap + afterWorkbookDispose/afterSheetDispose hooks). Reasons:

    The per-holder approach removes the lifetime and thread-safety concerns by construction — there is no shared mutable cache, so the design points about reference strength and synchronizedMap are moot.
    It avoids the handler-dispatch subtlety (a sheet-level handler does not receive afterWorkbookDispose through the workbook chain), keeping the change small and contained.
    It changes no public API.

    Regression test LongestMatchColumnWidthStyleStrategyTest covers both a control (separate instances) and the shared-instance leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions