Skip to content

GitHub Issue 1435: DataIterator leaks DB connection when import abort… #7919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-jeckels merged 2 commits into from
Aug 10, 2026
Merged

GitHub Issue 1435: DataIterator leaks DB connection when import abort… #7919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
181d588
GitHub Issue 1435: DataIterator leaks DB connection when import abort…
labkey-jeckels Aug 9, 2026
801abc6
Comments
labkey-jeckels Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions api/src/org/labkey/api/dataiterator/DataIteratorUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,15 @@
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

/**
* User: matthewb
* Date: 2011-05-31
* Time: 12:52 PM
* Static helpers for assembling DataIterator pipelines: matching a source iterator's columns to a target
* TableInfo's columns (by property URI, name, import alias, or JDBC-legal name) and copying or merging the
* result into that table. Also provides adapters that present an iterator as scrollable, map-based,
* map-transforming, or a Stream of maps.
*/
public class DataIteratorUtil
{
Expand Down Expand Up @@ -373,6 +375,28 @@ public static void closeQuietly(DataIterator it)
}
}

/**
* DataIteratorBuilder.getDataIterator() calls this to simplify implementing the success or close() input contract
*/
public static @Nullable DataIterator wrapOrClose(DataIteratorBuilder in, DataIteratorContext context, UnaryOperator<DataIterator> wrapper)

@labkey-matthewb labkey-matthewb Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataIteratorBuilder instead of UnaryOperator wrapper?

@labkey-matthewb labkey-matthewb Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I get it. If I weren't trying to minimize the diff, I'd probably rewrite this so that final AbstractDataIteratorBuilder.getDataIterator() did this wrapOrClose() work, and had a abstract DataIterator _getDataItertor().

I might make the comment describe the intended pattern. "DataIteratorBuilder.getDataIterator() calls this to simplify implementing the success or close() input contract."

@labkey-jeckels labkey-jeckels Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, and added a little top-level class JavaDoc too.

{
DataIterator di = in.getDataIterator(context);
if (null == di)
return null;

try
{
DataIterator out = wrapper.apply(di);
if (null == out)
closeQuietly(di);
return out;
}
catch (RuntimeException | Error e)
{
closeQuietly(di);
throw e;
}
}

/*
* Wrapping functions to add functionality to existing DataIterators
Expand Down
202 changes: 85 additions & 117 deletions experiment/src/org/labkey/experiment/ExpDataIterators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,60 +212,58 @@ public CounterDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container con
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _in.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors
return DataIteratorUtil.wrapOrClose(_in, context, pre -> {
SimpleTranslator counterTranslator = new SimpleTranslator(pre, context);
counterTranslator.setDebugName("Counter Def");
Set<String> skipColumns = new CaseInsensitiveHashSet();
Map<String, Integer> columnNameMap = DataIteratorUtil.createColumnNameMap(pre);

SimpleTranslator counterTranslator = new SimpleTranslator(pre, context);
counterTranslator.setDebugName("Counter Def");
Set<String> skipColumns = new CaseInsensitiveHashSet();
Map<String, Integer> columnNameMap = DataIteratorUtil.createColumnNameMap(pre);

for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions())
{
Set<String> attachedColumnNames = counterDefinition.getAttachedColumnNames();
skipColumns.addAll(attachedColumnNames);

// validate we have all the paired columns
List<Integer> pairedIndexes = new IntArrayList();
for (String pairedColumnName : counterDefinition.getPairedColumnNames())
for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions())
{
Integer i = columnNameMap.get(pairedColumnName);
if (i == null)
{
// immediately return error iterator tied to the input DataIterator instead of counterTranslator
ValidationException setupError = new ValidationException();
setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'");
return ErrorIterator.wrap(pre, context, true, setupError);
}
else
{
pairedIndexes.add(i);
}
}
Set<String> attachedColumnNames = counterDefinition.getAttachedColumnNames();
skipColumns.addAll(attachedColumnNames);

// add a sequence column for each of the attached columns
for (String columnName : attachedColumnNames)
{
Integer i = columnNameMap.get(columnName);
ColumnInfo column;
if (null != i)
// validate we have all the paired columns
List<Integer> pairedIndexes = new IntArrayList();
for (String pairedColumnName : counterDefinition.getPairedColumnNames())
{
column = pre.getColumnInfo(i);
skipColumns.add(columnName);
Integer i = columnNameMap.get(pairedColumnName);
if (i == null)
{
// immediately return error iterator tied to the input DataIterator instead of counterTranslator
ValidationException setupError = new ValidationException();
setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'");
return ErrorIterator.wrap(pre, context, true, setupError);
}
else
{
pairedIndexes.add(i);
}
}
else

// add a sequence column for each of the attached columns
for (String columnName : attachedColumnNames)
{
column = _expTable.getColumn(columnName);
}
Integer i = columnNameMap.get(columnName);
ColumnInfo column;
if (null != i)
{
column = pre.getColumnInfo(i);
skipColumns.add(columnName);
}
else
{
column = _expTable.getColumn(columnName);
}

counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100);
counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100);
}
}
}

counterTranslator.selectAll(skipColumns);
counterTranslator.selectAll(skipColumns);

return LoggingDataIterator.wrap(counterTranslator);
return LoggingDataIterator.wrap(counterTranslator);
});
}
}

Expand Down Expand Up @@ -347,11 +345,8 @@ public AliquotRollupDataIteratorBuilder(@NotNull DataIteratorBuilder in, Contain
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _in.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container));
return DataIteratorUtil.wrapOrClose(_in, context,
pre -> LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container)));
}
}

Expand Down Expand Up @@ -510,11 +505,8 @@ public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container conta
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator di = _in.getDataIterator(context);
if (di == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample));
return DataIteratorUtil.wrapOrClose(_in, context,
di -> LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample)));
}
}

Expand Down Expand Up @@ -631,11 +623,8 @@ public AutoLinkToStudyDataIteratorBuilder(@NotNull DataIteratorBuilder in, UserS
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _in.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType));
return DataIteratorUtil.wrapOrClose(_in, context,
pre -> LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType)));
}
}

Expand Down Expand Up @@ -754,11 +743,8 @@ public FlagDataIteratorBuilder(@NotNull DataIteratorBuilder in, User user, boole
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _in.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container));
return DataIteratorUtil.wrapOrClose(_in, context,
pre -> LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container)));
}
}

Expand Down Expand Up @@ -895,21 +881,19 @@ public DerivationDataIteratorBuilder(DataIteratorBuilder pre, Container containe
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator di = _pre.getDataIterator(context);
if (di == null)
return null; // can happen if context has errors
return DataIteratorUtil.wrapOrClose(_pre, context, di -> {
if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation))
return di;

if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation))
return di;

if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE)
di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot);
else if (_isSample)
di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);
else
di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);
if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE)
di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot);
else if (_isSample)
di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);
else
di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);

return LoggingDataIterator.wrap(di);
return LoggingDataIterator.wrap(di);
});
}
}

Expand Down Expand Up @@ -2124,11 +2108,8 @@ public SearchIndexIteratorBuilder(DataIteratorBuilder pre, Function<SearchIndexD
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _pre.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new SearchIndexIterator(pre, context, _indexFunction));
return DataIteratorUtil.wrapOrClose(_pre, context,
pre -> LoggingDataIterator.wrap(new SearchIndexIterator(pre, context, _indexFunction)));
}
}

Expand Down Expand Up @@ -2365,10 +2346,11 @@ public PersistDataIteratorBuilder setFileLinkDirectory(String dir)
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator input = _in.getDataIterator(context);
if (null == input)
return null; // Can happen if context has errors
return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context));
}

private DataIterator build(DataIterator input, DataIteratorContext context)
{
// useTransactionAuditCache already set for import and merge in AbstractQueryImportAction.createDataIteratorContext
if (context.getInsertOption() == QueryUpdateService.InsertOption.INSERT)
{
Expand Down Expand Up @@ -2536,26 +2518,21 @@ public SampleUpdateOnlyValidatorsIteratorBuilder(@NotNull DataIteratorBuilder in
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator di = _in.getDataIterator(context);
if (di == null)
return null; // can happen if context has errors

ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user);
Map<String, Integer> map = DataIteratorUtil.createColumnNameMap(validate);

Integer index = map.get(Name.name());
if (index != null)
{
ColumnInfo column = di.getColumnInfo(index);
validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank"));
}
return DataIteratorUtil.wrapOrClose(_in, context, di -> {
ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user);
Map<String, Integer> map = DataIteratorUtil.createColumnNameMap(validate);

// Add other column validators here...
Integer index = map.get(Name.name());
if (index != null)
{
ColumnInfo column = di.getColumnInfo(index);
validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank"));
}

if (validate.hasValidators())
di = validate;
// Add other column validators here...

return LoggingDataIterator.wrap(di);
return LoggingDataIterator.wrap(validate.hasValidators() ? validate : di);
});
}
}

Expand All @@ -2575,11 +2552,8 @@ public SampleNameChangeDataIteratorBuilder(@NotNull DataIteratorBuilder in, User
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator di = _in.getDataIterator(context);
if (di == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames));
return DataIteratorUtil.wrapOrClose(_in, context,
di -> LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames)));
}
}

Expand Down Expand Up @@ -3202,11 +3176,8 @@ public MultiDataTypeCrossProjectDataIteratorBuilder(@NotNull User user, @NotNull
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator di = _in.getDataIterator(context);
if (di == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples));
return DataIteratorUtil.wrapOrClose(_in, context,
di -> LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples)));
}
}

Expand All @@ -3232,11 +3203,8 @@ public SampleStatusCheckIteratorBuilder(@NotNull DataIteratorBuilder in, Contain
@Override
public DataIterator getDataIterator(DataIteratorContext context)
{
DataIterator pre = _in.getDataIterator(context);
if (pre == null)
return null; // can happen if context has errors

return LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container));
return DataIteratorUtil.wrapOrClose(_in, context,
pre -> LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container)));
}
}

Expand Down
2 changes: 2 additions & 0 deletions experiment/src/org/labkey/experiment/ExperimentModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
import org.labkey.experiment.api.ExperimentServiceImpl;
import org.labkey.experiment.api.ExperimentStressTest;
import org.labkey.experiment.api.GraphAlgorithms;
import org.labkey.experiment.api.ImportAbortResourceTestCase;
import org.labkey.experiment.api.LineageTest;
import org.labkey.experiment.api.LogDataType;
import org.labkey.experiment.api.Protocol;
Expand Down Expand Up @@ -1135,6 +1136,7 @@ public Collection<String> getSummary(Container c)
ExperimentServiceImpl.ParseInputOutputAliasTestCase.class,
ExperimentServiceImpl.TestCase.class,
ExperimentStressTest.class,
ImportAbortResourceTestCase.class,
LineagePerfTest.class,
LineageTest.class,
OntologyManager.TestCase.class,
Expand Down
8 changes: 5 additions & 3 deletions experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,10 +978,12 @@ private static boolean isReservedHeader(String name)
public DataIterator getDataIterator(DataIteratorContext context)
{
_context = context;
DataIterator input = _in.getDataIterator(context);
if (null == input)
return null; // Can happen if context has errors
return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context));
}

/** Returning null here (after adding an error) or throwing leaves `input` to be closed by wrapOrClose. */
private DataIterator build(DataIterator input, DataIteratorContext context)
{
boolean isMerge = context.getInsertOption() == QueryUpdateService.InsertOption.MERGE;
boolean isUpdate = context.getInsertOption() == QueryUpdateService.InsertOption.UPDATE;

Expand Down
Loading