Adding Close selector to prevent FD/FIFO leaks #3707

shvo123 · 2022-01-02T22:57:41Z

following File Descriptor leak issue #3705, we added slector.close() and this leak stopped. Closing/destroying ChannelOutputStream object does not close the selector therefore it retains redundant pipes/FD that cen be seen using lsof command or ls /proc/

following File Descriptor leak issue spring-projects#3705, we added slector.close() and this leak stopped. Closing/destroying ChannelOutputStream object does not close the selector therefore it retains redundant pipes/FD that cen be seen using lsof command or ls /proc/

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

set selector to null after closing it

artembilan · 2022-01-03T14:59:32Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

@@ -662,6 +662,8 @@ protected synchronized void doWrite(ByteBuffer buffer) throws IOException {
 				TcpNioConnection.this.socketChannel.write(buffer);
 				remaining = buffer.remaining();
 			}
+			selector.close();
+			selector = null;


And why do we need to close selector after each write?
You said yourself in the PR description:

Closing/destroying ChannelOutputStream object does not close the selector

I don't see that you close a ChannelOutputStream in this write operation, so I believe it is done somehow externally. Therefore that ChannelOutputStream.close() is called from there.
So, it still looks suspicious to always close a selector in the end of write, but not once when ChannelOutputStream is closed.

What do I miss, please?

OK I see your point it is better to move selector.close() to ChannelOutputStream.close()
And I would like to ask you if you feel comfortable with:
ChannelOutputStream.close() eventually calls doClose() that eventually calls super.close() that has the following code:
for (TcpSender sender : this.senders) { sender.removeDeadConnection(this);

This removeDeadConnection does nothing it has empty implementation, I feel (maybe wrong) that it may cause Out Of Memory in one of the maps in my case in TcpSendingMessageHandler in map connections.

I think that is not related to what we are talking here about.
I guess @garyrussell is looking into that via #3701 .

Let's concentrate here on the proper selector closing!

Make sure selector is closed to prevent FD leaks.

Close ChannelOutputStream and its selector to prevent leaks

shvo123 · 2022-01-03T16:14:29Z

Hi Artem, I added to doClose() close try { this.channelOutputStream.close(); } catch (@SuppressWarnings(UNUSED) Exception e) { }
not sure it is required what do you think ?

artembilan · 2022-01-03T16:46:40Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+				} catch (IOException e) {
+					// do nothing
+				}
+			}
 			doClose();


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.

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.

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() 😄

Make sure selector is closed to prevent FD/pipes leak

artembilan

The current code is probably OK so far in terms of context responsibility.
But I still wonder how and when this ChannelOutputStream.close() is called?

Your previous version was very close to the truth. We only needed to figure out what to do with the recursion.

I've just ran the whole test suite for ip module and no one calls this ChannelOutputStream.close(). Therefore your current fix does not bring us a desired final solution for original OOM issue...

/CC @garyrussell

garyrussell · 2022-01-03T20:31:34Z

Full diff now:

diff --git a/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java b/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java
index 1d1844c08f..4c529ba166 100644
--- a/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java
+++ b/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java
@@ -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.
@@ -139,6 +139,11 @@ public class TcpNioConnection extends TcpConnectionSupport {
 		}
 		catch (@SuppressWarnings(UNUSED) Exception e) {
 		}
+		try {
+			this.channelOutputStream.close();
+		}
+		catch (@SuppressWarnings(UNUSED) Exception e) {
+		}
 		super.close();
 	}
 
@@ -620,6 +625,13 @@ public class TcpNioConnection extends TcpConnectionSupport {
 
 		@Override
 		public void close() {
+			if (selector != null) {
+				try {
+					selector.close();
+				} catch (IOException e) {
+					// do nothing
+				}
+			}
 			doClose();
 		}
 
@@ -663,7 +675,6 @@ public class TcpNioConnection extends TcpConnectionSupport {
 				remaining = buffer.remaining();
 			}
 		}
-
 	}
 
 	/**

Not sure why GH is messed up.

artembilan · 2022-01-03T20:34:59Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+				} catch (IOException e) {
+					// do nothing
+				}
+			}
 			doClose();


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() 😄

Make sure OutputStream is closed to avoid FD leak

shvo123 · 2022-01-03T21:49:04Z

See last commit. I made sure no recursive call would take place.

shvo123 · 2022-01-03T21:50:54Z

I will try tomorrow to add unit test for ChannelOutputStream.close()

artembilan · 2022-01-03T21:54:40Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+		try {
+			// in order to prevent endless recursive calls from channelOutputStream.close() to doClose() that would call channelOutputStream.close() etc...
+			if (this.channelOutputStream != null) {
+				((OutputStream) this.channelOutputStream).close();


I don't understand how does your code help?
The this.channelOutputStream is never null according TcpNioConnection and you cannot set it to null (and you don't) because it is final.

And it is pointless to cast it to OutputStream since it is a super of the ChannelOutputStream.

Our idea was really to call this close here, but remove that doClose() from the ChannelOutputStream.

At the moment I'm very confused since the solution does not reflect your comments and does not address our discussion.

Please, elaborate what am I still missing?

Thanks

I took a look again in the code the source of the trouble is that doClose() does not call ChannelOutputStream.close(), and that the ChannelOutputStream.close() itself is erronous:
that is:
@Override public void close() { doClose(); }

doClose is not part of class ChannelOutputStream and what it does has nothing to do with ChannelOutputStream, it nor handle the selector that is private member of ChannelOutputStream neither closes the OutputStream itself.
So I think that the best approach would be to rewrite the ChannelOutputStream.close() to:
@Override public void close() { if (this.selector != null) { try { this.selector.close(); } catch (IOException e) { // do nothing } } super.close(); }
This is more similar to ChannelInputStream.close()
In addition I added ChannelOutputStream.close() to doClose().

In addition I took the liberty to write real impelmentation of ChannelOutputStream.flush()`.

Attached diff from the base source.

10x,
diff.txt

David

following File Descriptor leak issue spring-projects#3705, we added slector.close() and this leak stopped. Closing/destroying ChannelOutputStream object does not close the selector therefore it retains redundant pipes/FD that cen be seen using lsof command or ls /proc/

prevent FD leak by closing ChannelOutputStream and its selector. Rewriting doClose, ChannelOutputStream.close() and ChannelOutputStream.flush()

style correction

artembilan · 2022-01-04T14:32:29Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+				}
+			}
+			try {
+				super.close();


No reason to call super: it is just an empty method.
That's why we didn't delegate to it originally anyway.

artembilan · 2022-01-04T14:33:04Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

 		}

 		@Override
 		public void flush() {
+			try {
+				super.flush();


No reason to call super: it is just an empty method.

I think we can just remove this method altogether.

no problem will not override flush at all

artembilan · 2022-01-04T14:34:31Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+				try {
+					this.selector.close();
+				}
+				catch (@SuppressWarnings(UNUSED) Exception e) {


I don't think it is this method responsibility to suppress an exception.
Let it be done in the TcpNioConnection.close() instead!

I thought that we agreed in the previous discussion that this catch is going to be removed...

artembilan · 2022-01-04T14:35:54Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

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


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

not sure where this empty line is missing

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 !

artembilan · 2022-01-04T14:36:30Z

diff.txt

@@ -0,0 +1,51 @@
+diff --git a/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java b/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java


This file must not be here.

how can i remove it?

It must be somehow in your local project copy.
So, just remove it and push commit.

Removing flush, ChannelOutputStream.close() throws exception

artembilan · 2022-01-04T15:52:56Z

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

+				try {
+					this.selector.close();
+				}
+				catch (@SuppressWarnings(UNUSED) Exception e) {


I thought that we agreed in the previous discussion that this catch is going to be removed...

shvo123 · 2022-01-04T16:03:12Z

Quoting Artem, "I thought that we agreed in the previous discussion that this catch is going to be removed... "
I thought we are talking about super.close(), and I removed its catch clause accordingly. do not mind to remove catch clause of selector as well.
let me know about missing empty line location and then I would push it.

Remove redundant catch clause and redundant file

artembilan

It is OK with me so far.

Would you mind to add your name to the @author list of the affected class?
Also: any chances to cover this functionality with with some simple unit test?
I believe something like mocking with verification that ChannelOutputStream.close() is called when we call TcpNioConnection.close() should be enough.
Plus, confirm, please, that this change really fixes OOM problem for your project.

shvo123 · 2022-01-04T16:49:22Z

Would you mind to add your name to the @author list of the affected class?
like that?

@author Gary Russell
@author John Anderson
@author Artem Bilan
@SInCE 2.0
@author David Herschler Shvo
@SInCE 5.5.7

Also: any chances to cover this functionality with with some simple unit test?
I believe something like mocking with verification that ChannelOutputStream.close() is called when we call TcpNioConnection.close() should be enough.
Would try to add it to TcpNioConnectionWriteTests is it suitable?

Plus, confirm, please, that this change really fixes OOM problem for your project.

artembilan · 2022-01-04T16:55:51Z

Just add a new line @author David Herschler Shvo after all those.
You don't change @since because it is not a new class.

Yes, that TcpNioConnectionWriteTests looks like a good candidate indeed.

Probably something like ccf.getConnection() and call its close() should be enough and then you use TestUtils.getProperty(connection, 'channelOutputStream.selector', Selector.class) and check its status via isOpen(). Must be false. But not before this fix.

… failed Make sure internal output stream would close even if selector.close() failed by putting the outputStream.close(0 in finally clause

artembilan

I think that's it.
Taking this locally for final review and clean up.
Will try to add some testing as well...

shvo123 · 2022-01-05T19:04:50Z

10x

artembilan · 2022-01-05T22:08:49Z

Merged as and cherry-picked to 5.4.x & 5.3.x as 0b38b8d.

Sorry I couldn't figured out a simple test for this Selector use-case, so decided to merge it as is since the fix is fully straightforward with simple close() delegation which was missed before.

@shvo123 , thank you for contribution; looking forward for more!

shvo123 · 2022-01-06T06:18:14Z

Hi Artem, What you should do is to write a big buffer of 260K, in this situation `ChannelOutputStream.write()` would open selector and now without closing it you would have FD/PIPEs leak. After writing you can call: ```ChannelOutputStream cos = connection.getChannelOutputStream();``` ```TcpNioConnection.close();``` Now you need to check whether selector is still open, you can add isSelectorOpen method to ChannelOutputStream class, and call it. 10x, David

artembilan · 2022-01-06T14:22:43Z

Hi @shvo123 !

is to write a big buffer of 260K

Yes, I tried to do that, but it blocks when it tries to be selected.
Probably I miss something on the server side, because I believe the channel has to be released somehow for the selector.

If you can come up with some unit-test on the matter, I would appreciate your contribution.

Thanks

shvo123 · 2022-01-20T08:08:59Z

Hi Artem,
We tested the corrections made by Gary and they are OK.
The fix to close the selectors is also OK.

Can you tell me when formal release would be released so we can download it from maven repository instead of using our customized Spring integration.
10x,
David

garyrussell · 2022-01-20T14:28:52Z

5.5.8 (and the other supported branches) were released this week https://github.com/spring-projects/spring-integration/releases

shvo123 · 2022-01-20T14:41:12Z

10x

…

On Thu, Jan 20, 2022 at 4:29 PM Gary Russell ***@***.***> wrote: 5.5.8 (and the other supported branches) were released this week https://github.com/spring-projects/spring-integration/releases — Reply to this email directly, view it on GitHub <#3707 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOO3JESMJDC6ZJMDHGT6V3UXAL3BANCNFSM5LEFDYRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Rgds, David Herschler Shvo

Close selector

178b94b

following File Descriptor leak issue spring-projects#3705, we added slector.close() and this leak stopped. Closing/destroying ChannelOutputStream object does not close the selector therefore it retains redundant pipes/FD that cen be seen using lsof command or ls /proc/

artembilan requested changes Jan 3, 2022

View reviewed changes

...ion-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java Outdated Show resolved Hide resolved

Make sure selector is null after closing it

d78baf4

set selector to null after closing it

artembilan requested changes Jan 3, 2022

View reviewed changes

shvo123 added 2 commits January 3, 2022 17:17

make sure selector is closed to prevent FD leak

d3f94ec

Make sure selector is closed to prevent FD leaks.

Close ChannelOutputStream and its selector prevent leaks

5ffda69

Close ChannelOutputStream and its selector to prevent leaks

artembilan requested changes Jan 3, 2022

View reviewed changes

Make sure selector is closed to prevent FD/pipes leak

7794f3a

Make sure selector is closed to prevent FD/pipes leak

artembilan requested changes Jan 3, 2022

View reviewed changes

Make sure OutputStream is closed to avoid FD leak

71341c2

Make sure OutputStream is closed to avoid FD leak

artembilan requested changes Jan 3, 2022

View reviewed changes

shvo123 added 3 commits January 4, 2022 13:28

Close selector

409b3b1

following File Descriptor leak issue spring-projects#3705, we added slector.close() and this leak stopped. Closing/destroying ChannelOutputStream object does not close the selector therefore it retains redundant pipes/FD that cen be seen using lsof command or ls /proc/

prevent FD leak by closing ChannelOutputStreamre and its selector

4428d41

prevent FD leak by closing ChannelOutputStream and its selector. Rewriting doClose, ChannelOutputStream.close() and ChannelOutputStream.flush()

style correction

7b80ef3

style correction

artembilan requested changes Jan 4, 2022

View reviewed changes

Removing flush, ChannelOutputStream.close() throws exception

9175d0e

Removing flush, ChannelOutputStream.close() throws exception

artembilan requested changes Jan 4, 2022

View reviewed changes

Remove redundant catch clause and redundant file

e7703ff

Remove redundant catch clause and redundant file

artembilan requested changes Jan 4, 2022

View reviewed changes

Make sure internal output stream would close even if selector.close()…

76a166f

… failed Make sure internal output stream would close even if selector.close() failed by putting the outputStream.close(0 in finally clause

artembilan approved these changes Jan 5, 2022

View reviewed changes

artembilan closed this Jan 5, 2022

shvo123 deleted the closeSelector branch January 20, 2022 14:58

roma2341 mentioned this pull request Aug 8, 2024

spring-integration-sftp leads to sshd-SshClien threads leaks #9373

Closed

		@@ -0,0 +1,51 @@
		diff --git a/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java b/spring-integration-ip/src/main/java/org/springframework/integration/ip/tcp/connection/TcpNioConnection.java

Adding Close selector to prevent FD/FIFO leaks #3707

Adding Close selector to prevent FD/FIFO leaks #3707

Conversation

shvo123 commented Jan 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvo123 commented Jan 3, 2022

Choose a reason for hiding this comment

garyrussell Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

garyrussell commented Jan 3, 2022

Choose a reason for hiding this comment

shvo123 commented Jan 3, 2022 • edited Loading

shvo123 commented Jan 3, 2022

Choose a reason for hiding this comment

shvo123 Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvo123 commented Jan 4, 2022 • edited Loading

artembilan left a comment

Choose a reason for hiding this comment

shvo123 commented Jan 4, 2022

artembilan commented Jan 4, 2022

artembilan left a comment

Choose a reason for hiding this comment

shvo123 commented Jan 5, 2022

artembilan commented Jan 5, 2022

shvo123 commented Jan 6, 2022 via email • edited Loading

artembilan commented Jan 6, 2022

shvo123 commented Jan 20, 2022

garyrussell commented Jan 20, 2022

shvo123 commented Jan 20, 2022 via email

garyrussell Jan 3, 2022 •

edited

Loading

shvo123 commented Jan 3, 2022 •

edited

Loading

shvo123 Jan 4, 2022 •

edited

Loading

shvo123 commented Jan 4, 2022 •

edited

Loading

shvo123 commented Jan 6, 2022 via email •

edited

Loading