Skip to content

[TINKERPOP-2416] MultiIterator should implement AutoCloseable#1325

Merged
spmallette merged 1 commit intoapache:3.4-devfrom
rdtr:TINKERPOP-2416
Sep 14, 2020
Merged

[TINKERPOP-2416] MultiIterator should implement AutoCloseable#1325
spmallette merged 1 commit intoapache:3.4-devfrom
rdtr:TINKERPOP-2416

Conversation

@rdtr
Copy link
Copy Markdown
Contributor

@rdtr rdtr commented Sep 7, 2020

As described in the JIRA, currently MultiIterator does not implement AutoCloseable. This potentially introduces resource leak when we have multiple iterators that implement AutoCloseable and combine them using IteratorUtils#concat. The caller may expect all iterators to be closed automatically by calling CloseableIterator#closeIterator but right now MultiIterator doesn't implement AutoCloseable so even the underlying iterators are auto-closeable, they are never be closed.

This simple PR just adds AutoCloseable implementation to MultiIterator, close all inner iterators if they implement AutoCloseable.

@rdtr rdtr changed the title [TINKERPOP-2416] MultiIterator implement AutoCloseable and close hold… [TINKERPOP-2416] MultiIterator should implement AutoCloseable Sep 7, 2020
@spmallette
Copy link
Copy Markdown
Contributor

There are a number of places in TinkerPop code that handle MultiIterator instances. I guess we don't need to explicitly close() those as I don't think they hold expensive resources open (that I am aware of). Any thoughts on that?

@rdtr
Copy link
Copy Markdown
Contributor Author

rdtr commented Sep 9, 2020

I agree that we don't need to explicitly call close in TinkerPop's code.

The change is mainly for providers. When overriding TinkerPop's API to retrieve elements from their own internal Storage layer, if the returned iterator holds any reference which must be released in the end, it is the provider's responsibility to call close in their own method.

TinkerPop's base class and TinkerGraph doesn't have the resource issue so we don't need to call close there.

@spmallette
Copy link
Copy Markdown
Contributor

VOTE +1

@spmallette spmallette merged commit 06bae56 into apache:3.4-dev Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants