Skip to content

Commit

Permalink
Do not report exceptions on long running Excel reads
Browse files Browse the repository at this point in the history
This change introduces two modifications:
- `ClosedByInterruptException` is wrapped in `InterruptedException`
  instead of `RuntimeException`
- when instrumentation encounters `InterruptedException` it bails early

Having `ClosedByInterruptException` wrapped in `RuntimeException` meant
that it is being reported as a regular `HostException` in the engine and
to the user. Instead it should be treated specially since we know that
it is caused by cancelling a long-running job. Since it is a statically
checked exception it has to be declared and the information has to be
propagated through various lambda constructs (thanks Java!).

The above change alone meant that an error is not reported for
`Data.read` nodes but any values dependent on it would still report
`No_Such_Method` error when the exception is reported as a value. Hence
the early bail out mechanism.
  • Loading branch information
hubertp committed Dec 19, 2024
1 parent 269354e commit 5c8e588
Show file tree
Hide file tree
Showing 12 changed files with 182 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ object ProgramExecutionSupport {
val onComputedValueCallback: Consumer[ExpressionValue] = { value =>
if (callStack.isEmpty) {
logger.log(Level.FINEST, s"ON_COMPUTED ${value.getExpressionId}")

if (VisualizationResult.isInterruptedException(value.getValue)) {
value.getValue match {
case e: AbstractTruffleException =>
// Bail out early. Any references to the value will return `No_Such_Method` exception
throw new ThreadInterruptedException(e);
case _ =>
}
}
sendExpressionUpdate(contextId, executionFrame.syncState, value)
sendVisualizationUpdates(
contextId,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
package org.enso.interpreter.runtime.control;

/** Thrown when guest code discovers a thread interrupt. */
public class ThreadInterruptedException extends RuntimeException {}
public class ThreadInterruptedException extends RuntimeException {
public ThreadInterruptedException() {}

public ThreadInterruptedException(Throwable e) {
super(e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@
import org.apache.poi.ss.usermodel.Workbook;
import org.apache.poi.xssf.usermodel.XSSFWorkbook;
import org.enso.table.excel.xssfreader.XSSFReaderWorkbook;
import org.enso.table.util.FunctionWithException;

public class ExcelConnectionPool {
public static final ExcelConnectionPool INSTANCE = new ExcelConnectionPool();

private ExcelConnectionPool() {}

public ReadOnlyExcelConnection openReadOnlyConnection(File file, ExcelFileFormat format)
throws IOException {
throws IOException, InterruptedException {
synchronized (this) {
if (isCurrentlyWriting) {
throw new IllegalStateException(
Expand Down Expand Up @@ -131,7 +132,7 @@ public <R> R writeWorkbook(File file, Function<Workbook, R> writeAction) throws
*/
public <R> R lockForWriting(
File file, ExcelFileFormat format, File[] accompanyingFiles, Function<WriteHelper, R> action)
throws IOException {
throws IOException, InterruptedException {
synchronized (this) {
if (isCurrentlyWriting) {
throw new IllegalStateException(
Expand Down Expand Up @@ -216,7 +217,8 @@ static class ConnectionRecord {
private ExcelWorkbook workbook;
private IOException initializationException = null;

<T> T withWorkbook(Function<ExcelWorkbook, T> action) throws IOException {
<T> T withWorkbook(FunctionWithException<ExcelWorkbook, T, InterruptedException> action)
throws IOException, InterruptedException {
synchronized (this) {
return action.apply(accessCurrentWorkbook());
}
Expand All @@ -232,7 +234,7 @@ public void close() throws IOException {
}
}

void reopen(boolean throwOnFailure) throws IOException {
void reopen(boolean throwOnFailure) throws IOException, InterruptedException {
synchronized (this) {
if (workbook != null) {
throw new IllegalStateException("The workbook is already open.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ public static ExcelRange forRows(String sheetName, int topRow, int bottomRow) {
* @param sheet ExcelSheet containing the range refers to.
* @return Expanded range covering the connected table of cells.
*/
public static ExcelRange expandSingleCell(ExcelRange excelRange, ExcelSheet sheet) {
public static ExcelRange expandSingleCell(ExcelRange excelRange, ExcelSheet sheet)
throws InterruptedException {
ExcelRow currentRow = sheet.get(excelRange.getTopRow());
if (currentRow == null || currentRow.isEmpty(excelRange.getLeftColumn())) {
return new ExcelRange(
Expand Down Expand Up @@ -337,7 +338,7 @@ public int getRowCount() {
return isWholeColumn() ? Integer.MAX_VALUE : bottomRow - topRow + 1;
}

public int getLastNonEmptyRow(ExcelSheet sheet) {
public int getLastNonEmptyRow(ExcelSheet sheet) throws InterruptedException {
int lastRow =
Math.min(sheet.getLastRow(), isWholeColumn() ? sheet.getLastRow() : bottomRow) + 1;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,18 @@ public interface ExcelSheet {
String getName();

/** Gets the initial row index within the sheet (1-based). */
int getFirstRow();
int getFirstRow() throws InterruptedException;

/** Gets the final row index within the sheet (1-based). */
int getLastRow();
int getLastRow() throws InterruptedException;

/**
* Gets the row at the given index within the sheet (1-based)
*
* @param row the row index (1-based)/
* @return the row object or null if the row index is out of range or doesn't exist.
*/
ExcelRow get(int row);
ExcelRow get(int row) throws InterruptedException;

/** Gets the underlying Apache POI Sheet object - may be null. Provided for Writer use only. */
Sheet getSheet();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.enso.table.excel;

import java.io.IOException;
import java.util.function.Function;
import org.enso.table.util.FunctionWithException;

public class ReadOnlyExcelConnection implements AutoCloseable {

Expand All @@ -27,7 +27,9 @@ public synchronized void close() throws IOException {
record = null;
}

public synchronized <T> T withWorkbook(Function<ExcelWorkbook, T> f) throws IOException {
public synchronized <T> T withWorkbook(
FunctionWithException<ExcelWorkbook, T, InterruptedException> f)
throws IOException, InterruptedException {
if (record == null) {
throw new IllegalStateException("ReadOnlyExcelConnection is being used after it was closed.");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.enso.table.excel.xssfreader;

import java.io.IOException;
import java.nio.channels.ClosedByInterruptException;
import java.util.HashMap;
import java.util.Map;
import java.util.SortedMap;
Expand Down Expand Up @@ -33,7 +34,7 @@ public XSSFReaderSheet(int sheetIdx, String sheetName, String relId, XSSFReaderW
this.parent = parent;
}

private synchronized void ensureReadSheetData() {
private synchronized void ensureReadSheetData() throws InterruptedException {
if (hasReadSheetData) {
return;
}
Expand Down Expand Up @@ -70,6 +71,8 @@ protected void onCell(int rowNumber, short columnNumber, String ref, CellValue v
try {
var sheet = reader.getSheet(relId);
xmlReader.parse(new InputSource(sheet));
} catch (ClosedByInterruptException e) {
throw new InterruptedException(e.getMessage());
} catch (SAXException | InvalidFormatException | IOException e) {
throw new RuntimeException(e);
}
Expand All @@ -94,25 +97,25 @@ public String getName() {
return sheetName;
}

public String getDimensions() {
public String getDimensions() throws InterruptedException {
ensureReadSheetData();
return dimensions;
}

@Override
public int getFirstRow() {
public int getFirstRow() throws InterruptedException {
ensureReadSheetData();
return firstRow;
}

@Override
public int getLastRow() {
public int getLastRow() throws InterruptedException {
ensureReadSheetData();
return lastRow;
}

@Override
public ExcelRow get(int row) {
public ExcelRow get(int row) throws InterruptedException {
ensureReadSheetData();

if (!rowData.containsKey(row)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
package org.enso.table.excel.xssfreader;

import java.io.IOException;
import java.nio.channels.ClosedByInterruptException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
import javax.xml.XMLConstants;
import javax.xml.namespace.NamespaceContext;
import javax.xml.xpath.XPathConstants;
Expand All @@ -26,6 +26,7 @@
import org.apache.poi.xssf.usermodel.XSSFRelation;
import org.enso.table.excel.ExcelSheet;
import org.enso.table.excel.ExcelWorkbook;
import org.enso.table.util.ConsumerWithException;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
Expand Down Expand Up @@ -90,7 +91,7 @@ public Iterator<String> getPrefixes(String namespaceURI) {
private SharedStrings sharedStrings;
private XSSFReaderFormats styles;

public XSSFReaderWorkbook(String path) throws IOException {
public XSSFReaderWorkbook(String path) throws IOException, InterruptedException {
this.path = path;

// Read the workbook data
Expand All @@ -101,7 +102,8 @@ public String getPath() {
return path;
}

void withReader(Consumer<XSSFReader> action) throws IOException {
void withReader(ConsumerWithException<XSSFReader, InterruptedException> action)
throws IOException, InterruptedException {
try (var pkg = OPCPackage.open(path, PackageAccess.READ)) {
var reader = new XSSFReader(pkg);
action.accept(reader);
Expand All @@ -115,7 +117,7 @@ private record SheetInfo(int index, int sheetId, String name, String relID, bool

private record NamedRange(String name, String formula) {}

private void readWorkbookData() throws IOException {
private void readWorkbookData() throws IOException, InterruptedException {
withReader(
reader -> {
try {
Expand All @@ -124,6 +126,8 @@ private void readWorkbookData() throws IOException {
read1904DateSetting(workbookDoc);
readSheetInfo(workbookDoc);
readNamedRanges(workbookDoc);
} catch (ClosedByInterruptException e) {
throw new InterruptedException(e.getMessage());
} catch (SAXException
| IOException
| InvalidFormatException
Expand Down Expand Up @@ -171,7 +175,7 @@ private void read1904DateSetting(Document workbookDoc) throws XPathExpressionExc
}
}

private synchronized void ensureReadShared() {
private synchronized void ensureReadShared() throws InterruptedException {
if (hasReadShared) {
return;
}
Expand Down Expand Up @@ -207,6 +211,8 @@ public int getUniqueCount() {
styles = new XSSFReaderFormats(stylesTable);

hasReadShared = true;
} catch (ClosedByInterruptException e) {
throw new InterruptedException(e.getMessage());
} catch (InvalidFormatException | IOException e) {
throw new RuntimeException(e);
}
Expand Down Expand Up @@ -258,12 +264,12 @@ public String getNameFormula(String name) {
return namedRange == null ? null : namedRange.formula;
}

public SharedStrings getSharedStrings() {
public SharedStrings getSharedStrings() throws InterruptedException {
ensureReadShared();
return sharedStrings;
}

public XSSFReaderFormats getStyles() {
public XSSFReaderFormats getStyles() throws InterruptedException {
ensureReadShared();
return styles;
}
Expand Down
28 changes: 17 additions & 11 deletions std-bits/table/src/main/java/org/enso/table/read/ExcelReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.apache.poi.ss.util.CellReference;
Expand All @@ -24,6 +23,7 @@
import org.enso.table.excel.ExcelWorkbook;
import org.enso.table.excel.ReadOnlyExcelConnection;
import org.enso.table.problems.ProblemAggregator;
import org.enso.table.util.FunctionWithException;
import org.graalvm.polyglot.Context;

/** A table reader for MS Excel files. */
Expand All @@ -36,7 +36,8 @@ public class ExcelReader {
* @return a String[] containing the sheet names.
* @throws IOException when the action fails
*/
public static String[] readSheetNames(File file, ExcelFileFormat format) throws IOException {
public static String[] readSheetNames(File file, ExcelFileFormat format)
throws IOException, InterruptedException {
return withWorkbook(file, format, ExcelReader::readSheetNames);
}

Expand Down Expand Up @@ -65,7 +66,8 @@ public static String[] readSheetNames(ExcelWorkbook workbook) {
* @return a String[] containing the range names.
* @throws IOException when the action fails
*/
public static String[] readRangeNames(File file, ExcelFileFormat format) throws IOException {
public static String[] readRangeNames(File file, ExcelFileFormat format)
throws IOException, InterruptedException {
return withWorkbook(file, format, ExcelWorkbook::getRangeNames);
}

Expand All @@ -89,7 +91,7 @@ public static Table readSheetByName(
Integer row_limit,
ExcelFileFormat format,
ProblemAggregator problemAggregator)
throws IOException, InvalidLocationException {
throws IOException, InvalidLocationException, InterruptedException {
return withWorkbook(
file,
format,
Expand Down Expand Up @@ -130,7 +132,7 @@ public static Table readSheetByIndex(
Integer row_limit,
ExcelFileFormat format,
ProblemAggregator problemAggregator)
throws IOException, InvalidLocationException {
throws IOException, InvalidLocationException, InterruptedException {
return withWorkbook(
file,
format,
Expand Down Expand Up @@ -175,7 +177,7 @@ public static Table readRangeByName(
Integer row_limit,
ExcelFileFormat format,
ProblemAggregator problemAggregator)
throws IOException, InvalidLocationException {
throws IOException, InvalidLocationException, InterruptedException {
return withWorkbook(
file,
format,
Expand All @@ -202,7 +204,7 @@ public static Table readRangeByName(
int skip_rows,
Integer row_limit,
ProblemAggregator problemAggregator)
throws InvalidLocationException {
throws InvalidLocationException, InterruptedException {
int sheetIndex = workbook.getSheetIndex(rangeNameOrAddress);
if (sheetIndex != -1) {
return readTable(
Expand Down Expand Up @@ -247,7 +249,7 @@ public static Table readRange(
Integer row_limit,
ExcelFileFormat format,
ProblemAggregator problemAggregator)
throws IOException, InvalidLocationException {
throws IOException, InvalidLocationException, InterruptedException {
return withWorkbook(
file,
format,
Expand All @@ -256,7 +258,10 @@ public static Table readRange(
}

private static <T> T withWorkbook(
File file, ExcelFileFormat format, Function<ExcelWorkbook, T> action) throws IOException {
File file,
ExcelFileFormat format,
FunctionWithException<ExcelWorkbook, T, InterruptedException> action)
throws IOException, InterruptedException {
try (ReadOnlyExcelConnection connection =
ExcelConnectionPool.INSTANCE.openReadOnlyConnection(file, format)) {
return connection.withWorkbook(action);
Expand All @@ -270,7 +275,7 @@ public static Table readRange(
int skip_rows,
Integer row_limit,
ProblemAggregator problemAggregator)
throws InvalidLocationException {
throws InvalidLocationException, InterruptedException {
int sheetIndex = workbook.getSheetIndex(excelRange.getSheetName());
if (sheetIndex == -1) {
throw new InvalidLocationException(
Expand All @@ -294,7 +299,8 @@ private static Table readTable(
ExcelHeaders.HeaderBehavior headers,
int skipRows,
int rowCount,
ProblemAggregator problemAggregator) {
ProblemAggregator problemAggregator)
throws InterruptedException {

ExcelSheet sheet = workbook.getSheetAt(sheetIndex);

Expand Down
Loading

0 comments on commit 5c8e588

Please sign in to comment.