Skip to content

Close Input Streams in HDFS and Local Exchange storage#18606

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
sanchitkashyap:connection_leak
Aug 17, 2023
Merged

Close Input Streams in HDFS and Local Exchange storage#18606
losipiuk merged 1 commit intotrinodb:masterfrom
sanchitkashyap:connection_leak

Conversation

@sanchitkashyap
Copy link
Copy Markdown
Member

Description

When reading slices in HDFS and Local exchange storage, all input streams are not getting closed leading to connection leaks.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might be missing something obvious. sliceInput is closed here, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Every time getSliceInput is called, a new InputStreamSliceInput is created.. so we are not closing every input stream.

Copy link
Copy Markdown
Member

@linzebing linzebing Aug 10, 2023

Choose a reason for hiding this comment

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

Oh, then should we delete the code of:

if (sliceInput != null) {
                sliceInput.close();
                sliceInput = null;
}

? It becomes redundant now, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@sanchitkashyap sanchitkashyap force-pushed the connection_leak branch 2 times, most recently from 021d586 to 11079f8 Compare August 11, 2023 12:48
@sanchitkashyap
Copy link
Copy Markdown
Member Author

The failing tests look unrelated.
@linzebing Can we merge this change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like we do not need to accumulate all the sliceInputs in closer.
Instead we can do change

            if (sliceInput != null && sliceInput.isReadable()) {
                return sliceInput.readSlice(sliceInput.readInt());
            }

to

            if (sliceInput != null) {
                if (sliceInput.isReadable()) {
                    return sliceInput.readSlice(sliceInput.readInt());
                }
                else {
                    sliceInput.close();
                }
            }

@losipiuk losipiuk merged commit a6a213c into trinodb:master Aug 17, 2023
@github-actions github-actions bot added this to the 424 milestone Aug 17, 2023
@hackeryang hackeryang added the bug label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants