diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java index 14ed82bf2..459b2d004 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java @@ -233,6 +233,13 @@ public void finish() { } catch (Throwable t) { throwable = t; } + try { + if (excelReadExecutor != null) { + excelReadExecutor.close(); + } + } catch (Throwable t) { + throwable = t; + } try { if ((readWorkbookHolder instanceof XlsxReadWorkbookHolder) && ((XlsxReadWorkbookHolder) readWorkbookHolder).getOpcPackage() != null) { diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java index d7700fca0..41883289d 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelReadExecutor.java @@ -46,4 +46,12 @@ public interface ExcelReadExecutor { * Read the sheet. */ void execute(); + + /** + * Release resources held by this executor. Invoked when the reader is finished. + * + *

XLSX sheet streams are opened up front and may be consumed across multiple + * {@link #execute()} calls, so leftover streams must not be closed until this method runs. + */ + default void close() {} } diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java index b65048ebf..4bdb30547 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyser.java @@ -149,6 +149,7 @@ public XlsxSaxAnalyser(XlsxReadContext xlsxReadContext, InputStream decryptedStr String sheetName = ite.getSheetName(); CTSheet ctSheet = ctSheetMap.get(sheetName); if (ctSheet == null) { + closeSheetInputStream(inputStream, "sheetName=" + sheetName); continue; } ReadSheet readSheet = new ReadSheet(index, sheetName); @@ -204,14 +205,15 @@ private void analysisUse1904WindowDate(XSSFReader xssfReader, XlsxReadWorkbookHo if (xlsxReadWorkbookHolder.getReadWorkbook().getUse1904windowing() != null) { return; } - InputStream workbookXml = xssfReader.getWorkbookData(); - WorkbookDocument ctWorkbook = WorkbookDocument.Factory.parse(workbookXml); - CTWorkbook wb = ctWorkbook.getWorkbook(); - CTWorkbookPr prefix = wb.getWorkbookPr(); - if (prefix != null && prefix.getDate1904()) { - xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.TRUE); - } else { - xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.FALSE); + try (InputStream workbookXml = xssfReader.getWorkbookData()) { + WorkbookDocument ctWorkbook = WorkbookDocument.Factory.parse(workbookXml); + CTWorkbook wb = ctWorkbook.getWorkbook(); + CTWorkbookPr prefix = wb.getWorkbookPr(); + if (prefix != null && prefix.getDate1904()) { + xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.TRUE); + } else { + xlsxReadWorkbookHolder.getGlobalConfiguration().setUse1904windowing(Boolean.FALSE); + } } } @@ -224,13 +226,14 @@ private void analysisSharedStringsTable( private void analysisCtSheetMap(XSSFReader xssfReader, XlsxReadWorkbookHolder xlsxReadWorkbookHolder) throws Exception { - CTWorkbook wb = - WorkbookDocument.Factory.parse(xssfReader.getWorkbookData()).getWorkbook(); - for (CTSheet ctSheet : wb.getSheets().getSheetList()) { - boolean isHidden = - (ctSheet.getState() == STSheetState.HIDDEN) || (ctSheet.getState() == STSheetState.VERY_HIDDEN); - if (Boolean.FALSE.equals(xlsxReadWorkbookHolder.getIgnoreHiddenSheet()) || !isHidden) { - ctSheetMap.put(ctSheet.getName(), ctSheet); + try (InputStream workbookXml = xssfReader.getWorkbookData()) { + CTWorkbook wb = WorkbookDocument.Factory.parse(workbookXml).getWorkbook(); + for (CTSheet ctSheet : wb.getSheets().getSheetList()) { + boolean isHidden = + (ctSheet.getState() == STSheetState.HIDDEN) || (ctSheet.getState() == STSheetState.VERY_HIDDEN); + if (Boolean.FALSE.equals(xlsxReadWorkbookHolder.getIgnoreHiddenSheet()) || !isHidden) { + ctSheetMap.put(ctSheet.getName(), ctSheet); + } } } } @@ -313,13 +316,13 @@ private void parseXmlSource(InputStream inputStream, ContentHandler handler) { @Override public void execute() { for (ReadSheet readSheet : sheetList) { - readSheet = SheetUtils.match(readSheet, xlsxReadContext); - if (readSheet != null) { + ReadSheet matchedSheet = SheetUtils.match(readSheet, xlsxReadContext); + if (matchedSheet != null) { try { - xlsxReadContext.currentSheet(readSheet); - parseXmlSource(sheetMap.get(readSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); + xlsxReadContext.currentSheet(matchedSheet); + parseXmlSource(sheetMap.get(matchedSheet.getSheetNo()), new XlsxRowHandler(xlsxReadContext)); // Read comments - readComments(readSheet); + readComments(matchedSheet); } catch (ExcelAnalysisStopSheetException e) { if (log.isDebugEnabled()) { log.debug("Custom stop!", e); @@ -331,6 +334,31 @@ public void execute() { } } + @Override + public void close() { + closeRemainingSheetStreams(); + } + + private void closeRemainingSheetStreams() { + for (Map.Entry entry : sheetMap.entrySet()) { + closeSheetInputStream(entry.getValue(), "sheetNo=" + entry.getKey()); + } + } + + /** + * Package-private so tests can assert skipped-sheet streams are closed. + */ + void closeSheetInputStream(InputStream inputStream, String description) { + if (inputStream == null) { + return; + } + try { + inputStream.close(); + } catch (IOException e) { + log.warn("Failed to close sheet input stream, {}", description, e); + } + } + private void readComments(ReadSheet readSheet) { if (!xlsxReadContext.readWorkbookHolder().getExtraReadSet().contains(CellExtraTypeEnum.COMMENT)) { return; diff --git a/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java b/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java index 6084aa32b..a170587ac 100644 --- a/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java +++ b/fesod-sheet/src/main/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelector.java @@ -26,6 +26,7 @@ package org.apache.fesod.sheet.cache.selector; import java.io.IOException; +import java.io.InputStream; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.Setter; @@ -98,8 +99,8 @@ public SimpleReadCacheSelector(Long maxUseMapCacheSize, Integer maxCacheActivate public ReadCache readCache(PackagePart sharedStringsTablePackagePart) { long size = sharedStringsTablePackagePart.getSize(); if (size < 0) { - try { - size = sharedStringsTablePackagePart.getInputStream().available(); + try (InputStream inputStream = sharedStringsTablePackagePart.getInputStream()) { + size = inputStream.available(); } catch (IOException e) { log.warn("Unable to get file size, default used MapCache"); return new MapCache(); diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java new file mode 100644 index 000000000..7e8e2093e --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/XlsxSaxAnalyserSheetStreamCloseTest.java @@ -0,0 +1,303 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.analysis.v07; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.fesod.sheet.ExcelReader; +import org.apache.fesod.sheet.ExcelWriter; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.context.AnalysisContext; +import org.apache.fesod.sheet.context.xlsx.DefaultXlsxReadContext; +import org.apache.fesod.sheet.context.xlsx.XlsxReadContext; +import org.apache.fesod.sheet.event.AnalysisEventListener; +import org.apache.fesod.sheet.read.metadata.ReadSheet; +import org.apache.fesod.sheet.read.metadata.ReadWorkbook; +import org.apache.fesod.sheet.support.ExcelTypeEnum; +import org.apache.fesod.sheet.testkit.Tags; +import org.apache.fesod.sheet.testkit.base.AbstractExcelTest; +import org.apache.fesod.sheet.testkit.builders.TestDataBuilder; +import org.apache.fesod.sheet.testkit.enums.ExcelFormat; +import org.apache.fesod.sheet.testkit.listeners.CollectingReadListener; +import org.apache.fesod.sheet.testkit.models.SimpleData; +import org.apache.fesod.sheet.util.FileUtils; +import org.apache.fesod.sheet.write.metadata.WriteSheet; +import org.apache.poi.xssf.usermodel.XSSFWorkbook; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Verifies that {@link XlsxSaxAnalyser} closes sheet {@link InputStream}s that are skipped or left unread. + */ +@Tag(Tags.READ) +@Tag(Tags.ROUND_TRIP) +class XlsxSaxAnalyserSheetStreamCloseTest extends AbstractExcelTest { + + @Test + void execute_keepsUnreadSheetStreamsOpen_untilReaderCloses() throws Exception { + File file = writeThreeSheets(); + CollectingReadListener listener = new CollectingReadListener<>(); + Map tracked = null; + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, listener).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + Map sheetMap = sheetMap(analyser); + Assertions.assertEquals(3, sheetMap.size()); + + tracked = wrapSheetStreams(sheetMap); + excelReader.read(FesodSheet.readSheet(0).build()); + + Assertions.assertEquals(1, listener.getRowCount()); + Assertions.assertEquals("sheet1", listener.getFirstRow().getName()); + Assertions.assertTrue(tracked.get(0).isClosed()); + Assertions.assertFalse(tracked.get(1).isClosed()); + Assertions.assertFalse(tracked.get(2).isClosed()); + + excelReader.read(FesodSheet.readSheet(1).build()); + Assertions.assertEquals(2, listener.getRowCount()); + Assertions.assertTrue(tracked.get(1).isClosed()); + Assertions.assertFalse(tracked.get(2).isClosed()); + } + + Assertions.assertNotNull(tracked); + assertAllClosed(tracked); + } + + @Test + void execute_closesRemainingSheetStreams_whenListenerThrows() throws Exception { + File file = writeThreeSheets(); + AnalysisEventListener failingListener = new AnalysisEventListener() { + @Override + public void invoke(SimpleData data, AnalysisContext context) { + throw new IllegalStateException("boom"); + } + + @Override + public void doAfterAllAnalysed(AnalysisContext context) { + // ignore code + } + }; + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, failingListener).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + Map tracked = wrapSheetStreams(sheetMap(analyser)); + + Assertions.assertThrows(IllegalStateException.class, excelReader::readAll); + assertAllClosed(tracked); + } + } + + @Test + void sequentialSheetReads_doNotCloseLaterSheetsEarly() { + File file = writeThreeSheets(); + CollectingReadListener listener = new CollectingReadListener<>(); + + try (ExcelReader excelReader = + FesodSheet.read(file, SimpleData.class, listener).build()) { + excelReader.read(FesodSheet.readSheet(0).build()); + excelReader.read(FesodSheet.readSheet(1).build()); + excelReader.read(FesodSheet.readSheet(2).build()); + } + + Assertions.assertEquals(3, listener.getRowCount()); + Assertions.assertEquals("sheet1", listener.getRows().get(0).getName()); + Assertions.assertEquals("sheet2", listener.getRows().get(1).getName()); + Assertions.assertEquals("sheet3", listener.getRows().get(2).getName()); + } + + @Test + void constructor_doesNotRetainHiddenSheetStreams_whenIgnoreHiddenSheet() throws Exception { + File file = writeWorkbookWithHiddenSheet(); + + try (ExcelReader excelReader = + FesodSheet.read(file).ignoreHiddenSheet(Boolean.TRUE).build()) { + XlsxSaxAnalyser analyser = (XlsxSaxAnalyser) excelReader.excelExecutor(); + List sheets = analyser.sheetList(); + Map sheetMap = sheetMap(analyser); + + Assertions.assertEquals(2, sheets.size()); + Assertions.assertEquals(2, sheetMap.size()); + Assertions.assertFalse(containsSheetName(sheets, "Hidden")); + Assertions.assertTrue(containsSheetName(sheets, "Visible1")); + Assertions.assertTrue(containsSheetName(sheets, "Visible2")); + + Map tracked = wrapSheetStreams(sheetMap); + excelReader.readAll(); + assertAllClosed(tracked); + } + } + + @Test + void constructor_closesSkippedHiddenSheetStream_whenIgnoreHiddenSheet() throws Exception { + File file = writeWorkbookWithHiddenSheet(); + ReadWorkbook readWorkbook = new ReadWorkbook(); + readWorkbook.setFile(file); + readWorkbook.setIgnoreHiddenSheet(Boolean.TRUE); + XlsxReadContext context = new DefaultXlsxReadContext(readWorkbook, ExcelTypeEnum.XLSX); + TrackingXlsxSaxAnalyser analyser = new TrackingXlsxSaxAnalyser(context); + try { + Assertions.assertFalse(containsSheetName(analyser.sheetList(), "Hidden")); + boolean hiddenClosed = false; + for (String description : analyser.closedDescriptions()) { + if (description != null && description.contains("Hidden")) { + hiddenClosed = true; + break; + } + } + Assertions.assertTrue(hiddenClosed, "Skipped hidden sheet stream should be closed during construction"); + } finally { + analyser.close(); + if (context.xlsxReadWorkbookHolder().getOpcPackage() != null) { + context.xlsxReadWorkbookHolder().getOpcPackage().revert(); + } + if (context.xlsxReadWorkbookHolder().getReadCache() != null) { + context.xlsxReadWorkbookHolder().getReadCache().destroy(); + } + if (context.xlsxReadWorkbookHolder().getTempFile() != null) { + FileUtils.delete(context.xlsxReadWorkbookHolder().getTempFile()); + } + } + } + + private File writeThreeSheets() { + File file = createTempFileUnchecked(); + try (ExcelWriter excelWriter = FesodSheet.write(file, SimpleData.class).build()) { + WriteSheet sheet1 = FesodSheet.writerSheet(0, "Sheet1").build(); + WriteSheet sheet2 = FesodSheet.writerSheet(1, "Sheet2").build(); + WriteSheet sheet3 = FesodSheet.writerSheet(2, "Sheet3").build(); + excelWriter.write(namedData("sheet1"), sheet1); + excelWriter.write(namedData("sheet2"), sheet2); + excelWriter.write(namedData("sheet3"), sheet3); + } + return file; + } + + private File writeWorkbookWithHiddenSheet() throws IOException { + File file = createTempFileUnchecked(); + try (XSSFWorkbook workbook = new XSSFWorkbook()) { + workbook.createSheet("Visible1").createRow(0).createCell(0).setCellValue("a"); + workbook.createSheet("Hidden").createRow(0).createCell(0).setCellValue("b"); + workbook.createSheet("Visible2").createRow(0).createCell(0).setCellValue("c"); + workbook.setSheetHidden(1, true); + try (FileOutputStream outputStream = new FileOutputStream(file)) { + workbook.write(outputStream); + } + } + return file; + } + + private File createTempFileUnchecked() { + try { + return createTempFile(ExcelFormat.XLSX); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } + + private static List namedData(String name) { + List data = TestDataBuilder.simpleData(1); + data.get(0).setName(name); + return data; + } + + private static boolean containsSheetName(List sheets, String sheetName) { + for (ReadSheet sheet : sheets) { + if (sheetName.equals(sheet.getSheetName())) { + return true; + } + } + return false; + } + + @SuppressWarnings("unchecked") + private static Map sheetMap(XlsxSaxAnalyser analyser) throws Exception { + Field field = XlsxSaxAnalyser.class.getDeclaredField("sheetMap"); + field.setAccessible(true); + return (Map) field.get(analyser); + } + + private static Map wrapSheetStreams(Map sheetMap) { + Map tracked = new HashMap<>(); + for (Map.Entry entry : sheetMap.entrySet()) { + CloseTrackingInputStream wrapped = new CloseTrackingInputStream(entry.getValue()); + tracked.put(entry.getKey(), wrapped); + sheetMap.put(entry.getKey(), wrapped); + } + return tracked; + } + + private static void assertAllClosed(Map tracked) { + Assertions.assertFalse(tracked.isEmpty()); + for (Map.Entry entry : tracked.entrySet()) { + Assertions.assertTrue( + entry.getValue().isClosed(), "Sheet stream should be closed, sheetNo=" + entry.getKey()); + } + } + + private static final class CloseTrackingInputStream extends FilterInputStream { + private boolean closed; + + private CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() throws IOException { + closed = true; + super.close(); + } + + private boolean isClosed() { + return closed; + } + } + + private static final class TrackingXlsxSaxAnalyser extends XlsxSaxAnalyser { + private List closedDescriptions; + + private TrackingXlsxSaxAnalyser(XlsxReadContext xlsxReadContext) throws Exception { + super(xlsxReadContext, null); + } + + private List closedDescriptions() { + if (closedDescriptions == null) { + closedDescriptions = new ArrayList<>(); + } + return closedDescriptions; + } + + @Override + void closeSheetInputStream(InputStream inputStream, String description) { + closedDescriptions().add(description); + super.closeSheetInputStream(inputStream, description); + } + } +} diff --git a/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java b/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java new file mode 100644 index 000000000..5667a5959 --- /dev/null +++ b/fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/selector/SimpleReadCacheSelectorTest.java @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.sheet.cache.selector; + +import java.io.ByteArrayInputStream; +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; +import org.apache.fesod.sheet.cache.MapCache; +import org.apache.fesod.sheet.cache.ReadCache; +import org.apache.fesod.sheet.testkit.Tags; +import org.apache.poi.openxml4j.opc.PackagePart; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Tests {@link SimpleReadCacheSelector}. + */ +@Tag(Tags.UNIT) +@ExtendWith(MockitoExtension.class) +class SimpleReadCacheSelectorTest { + + @Test + void readCache_closesInputStream_whenPackagePartSizeUnknown() throws Exception { + PackagePart packagePart = Mockito.mock(PackagePart.class); + CloseTrackingInputStream inputStream = + new CloseTrackingInputStream(new ByteArrayInputStream(new byte[] {1, 2, 3, 4})); + Mockito.when(packagePart.getSize()).thenReturn(-1L); + Mockito.when(packagePart.getInputStream()).thenReturn(inputStream); + + ReadCache cache = new SimpleReadCacheSelector().readCache(packagePart); + + Assertions.assertTrue(inputStream.isClosed()); + Assertions.assertInstanceOf(MapCache.class, cache); + } + + @Test + void readCache_doesNotOpenInputStream_whenPackagePartSizeIsKnown() throws Exception { + PackagePart packagePart = Mockito.mock(PackagePart.class); + Mockito.when(packagePart.getSize()).thenReturn(1024L); + + ReadCache cache = new SimpleReadCacheSelector().readCache(packagePart); + + Mockito.verify(packagePart, Mockito.never()).getInputStream(); + Assertions.assertInstanceOf(MapCache.class, cache); + } + + private static final class CloseTrackingInputStream extends FilterInputStream { + private boolean closed; + + private CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() throws IOException { + closed = true; + super.close(); + } + + private boolean isClosed() { + return closed; + } + } +}