GitHub Issue 1435: DataIterator leaks DB connection when import abort… - #7919
Conversation
…s during construction
| * Builds `in` and hands it to `wrapper`, propagating a null input (which means the context has errors). If the | ||
| * wrapper throws or returns null, the built input is closed rather than abandoned/leaked. | ||
| */ | ||
| public static @Nullable DataIterator wrapOrClose(DataIteratorBuilder in, DataIteratorContext context, UnaryOperator<DataIterator> wrapper) |
There was a problem hiding this comment.
DataIteratorBuilder instead of UnaryOperator wrapper?
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Updated, and added a little top-level class JavaDoc too.
| * Builds `in` and hands it to `wrapper`, propagating a null input (which means the context has errors). If the | ||
| * wrapper throws or returns null, the built input is closed rather than abandoned/leaked. | ||
| */ | ||
| public static @Nullable DataIterator wrapOrClose(DataIteratorBuilder in, DataIteratorContext context, UnaryOperator<DataIterator> wrapper) |
There was a problem hiding this comment.
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."
2 participants
Rationale
We're leaking a DB connection (eventually cleaned up on garbage collection) when DataIterator construction bails out due to a problem like duplicate or unwanted columns.
Changes
wrapOrClose()method to centralize closing when we bail out on a handled or unhandled errorDataIteratorimplementations to use helper