Skip to content

XlsDataSet opens workbooks via the POI InputStream overload, buffering the whole document #782

Description

@jeffjensen

Problem

The XlsDataSet constructor (~lines 67, 73–79) opens workbooks with WorkbookFactory.create(InputStream) over an unbuffered FileInputStream. POI buffers the entire document in memory when reading from a stream and recommends the File overload.

Status: still unresolved. Investigated as part of WI-23 in performance-improvements.adoc; the originally-proposed fix turned out not to be safe as specified. Full reasoning is in that document's "Not Completed" section.

What was investigated

The proposed fix's own gate said: only switch to WorkbookFactory.create(File) if the workbook is fully read and closed within the XlsDataSet constructor — otherwise skip, because leaving a Workbook open holds a Windows file lock indefinitely.

XlsTable (the ITable XlsDataSet builds per sheet) keeps a reference to the POI Sheet and reads cell values lazily in getValue(), for the table's entire lifetime — not just during construction. Closing the Workbook inside the constructor would break every getValue() call made afterward. The gate's condition isn't met, so the File-overload switch was not made; XlsDataSet(File) still calls WorkbookFactory.create(InputStream) and POI still buffers the whole document in memory exactly as described above.

A separate, unrelated bug was found and fixed along the way: XlsDataSet(File)'s own FileInputStream was never closed at all (a plain resource leak, independent of which WorkbookFactory overload is used). That's tracked and resolved as #788 — it does not address this issue.

Remaining fix need

The core problem (POI buffering the whole document in memory) is still present. A real fix has to account for XlsTable's lazy, whole-lifetime dependency on the Sheet. Two directions, neither attempted:

  1. Switch to WorkbookFactory.create(File) but keep the Workbook open for XlsDataSet/XlsTable's full lifetime instead of closing it in the constructor. Trades the memory win for holding a file handle open indefinitely — needs its own review of Windows file-locking impact on callers.
  2. Make XlsTable eager — read all cell values into an in-memory structure at construction instead of lazily reading from the Sheet per getValue() call — so the Workbook genuinely can be closed in the constructor. A materially larger, more invasive change to XlsTable's design than this issue originally scoped.

Proposed fix (original, superseded by the above)

Use WorkbookFactory.create(File) for file-based construction. Gated: only if the workbook is fully read and closed within the constructor — otherwise skip (Windows file locking).

Activity

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

    area: datasetIDataSet, ITable, Column, dataset implementationsformat: xlsXlsDataSet (Apache POI)type: performancePerformance improvement

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions