Skip to content

Adding Close selector to prevent FD/FIFO leaks #3707

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

Closed
wants to merge 12 commits into from
Closed

Adding Close selector to prevent FD/FIFO leaks #3707

Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
178b94b
Close selector
shvo123 Jan 2, 2022
d78baf4
Make sure selector is null after closing it
shvo123 Jan 3, 2022
d3f94ec
make sure selector is closed to prevent FD leak
shvo123 Jan 3, 2022
5ffda69
Close ChannelOutputStream and its selector prevent leaks
shvo123 Jan 3, 2022
7794f3a
Make sure selector is closed to prevent FD/pipes leak
shvo123 Jan 3, 2022
71341c2
Make sure OutputStream is closed to avoid FD leak
shvo123 Jan 3, 2022
409b3b1
Close selector
shvo123 Jan 4, 2022
4428d41
prevent FD leak by closing ChannelOutputStreamre and its selector
shvo123 Jan 4, 2022
7b80ef3
style correction
shvo123 Jan 4, 2022
9175d0e
Removing flush, ChannelOutputStream.close() throws exception
shvo123 Jan 4, 2022
e7703ff
Remove redundant catch clause and redundant file
shvo123 Jan 4, 2022
76a166f
Make sure internal output stream would close even if selector.close()…
shvo123 Jan 5, 2022
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
11 changes: 9 additions & 2 deletions ...-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -620,6 +620,14 @@ public void write(int b) throws IOException {

@Override
public void close() {
if (this.selector != null) {
try {
this.selector.close();
}
catch (IOException e) {
// do nothing
}
}
doClose();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think such an implementation is OK.
This doClose() is exactly that one in the outer class, so this close() is going to be called again.
And so on in recursive manner.

I'm not sure what was an original intention with such a delegation, but we need to be sure that we don't do a recursion.

Could you, please, double check the solution?
Also the catch block must be on a new line.
Be sure run \gradlew clean :spring-integration-ip:check before pushing the change to the PR.

Copy link
Contributor

@garyrussell garyrussell Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure what's going on, but GitHub is not rendering the changes from all 5 commits; I can only see this change.

It looks like a bug that we never closed the output stream; we would have hit the stack overflow problem if we did; day one issue, I think.

I believe we can just remove that doClose() call here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and since the output close() is never called from the outside, it is safe to have it call it from connection close.
And yes: still keep a selector.close() 😄

}

Expand Down Expand Up @@ -663,7 +671,6 @@ protected synchronized void doWrite(ByteBuffer buffer) throws IOException {
remaining = buffer.remaining();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This is wrong: every member (even last method) of the class has to be surrounded with blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure where this empty line is missing

Copy link
Member

Choose a reason for hiding this comment

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

Just bring it back and push!
It is not a problem though: we can clean it up on merge.
Just letting you to know what is our code style: https://github.com/spring-projects/spring-integration/wiki/Spring-Integration-Framework-Code-Style !

}

/**
Expand Down