Skip to content
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.

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

Close Sftp Session before exception is thrown ? #3827

Closed
corentin-pasquier opened this issue Jun 24, 2022 · 1 comment · Fixed by #3853
Closed

Close Sftp Session before exception is thrown ? #3827

corentin-pasquier opened this issue Jun 24, 2022 · 1 comment · Fixed by #3853
Assignees
Milestone

Comments

@corentin-pasquier
Copy link

Currently using Spring Integration 5.5.9 but the code looks the same on the main branch.

Describe the bug

Our application is creating sftp connections that stay opened and are never released to the connection pool.
After some time the pool is empty and we get an error "Timed out while waiting to acquire a pool entry".

For instance, this a a view of the sftp connection for the last 2 days :
image

After some digging I found that ths sftp connection is never closed when an exception is thrown at this moment :
https://github.com/spring-projects/spring-integration/blob/main/spring-integration-file/src/main/java/org/springframework/integration/file/remote/gateway/AbstractRemoteFileOutboundGateway.java#L667

To Reproduce

First you need a sftp server.
Then you need a SessionFactory (this is our configuration) :

	<bean id="sftpSessionFactory"
		class="org.springframework.integration.file.remote.session.CachingSessionFactory">
		<constructor-arg ref="defaultSftpSessionFactory" />
		<property name="poolSize" value="${application.sftp.pool-size}"/>
        	<property name="sessionWaitTimeout" value="${application.sftp.session-timeout}"/>
        	<property name="testSession" value="true"/>
	</bean>

	<bean id="defaultSftpSessionFactory" 
		class="org.springframework.integration.sftp.session.DefaultSftpSessionFactory">
		<property name="host" value="${application.sftp.host}" />
		<property name="port" value="${application.sftp.port}" />
		<property name="user" value="${application.sftp.user}" />
		<property name="password" value="${application.sftp.password}" />
		<property name="allowUnknownKeys" value="true" />
	</bean>

And a sftp-outbound-gateway with a get command and a -stream option :

		<int-sftp:outbound-gateway id="getFileFromSftp"
			session-factory="sftpSessionFactory"
			command="get"
			command-options="-stream"
			reply-timeout="15000"
			expression="headers['pdfFileName']">
		</int-sftp:outbound-gateway>

When you try to get a file that doens't exist, the session stays opened.

Expected behavior

Close the sftp connection before throwing the ErrorMessage so it doesn't stay opened.

I was thinking something like this (maybe adding a non-null verification or other stuff would be better) :

			session = this.remoteFileTemplate.getSessionFactory().getSession();
			try {
				payload = session.readRaw(remoteFilePath);
				return getMessageBuilderFactory()
						.withPayload(payload)
						.setHeader(FileHeaders.REMOTE_DIRECTORY, remoteDir)
						.setHeader(FileHeaders.REMOTE_FILE, remoteFilename)
						.setHeader(FileHeaders.REMOTE_HOST_PORT, session.getHostPort())
						.setHeader(IntegrationMessageHeaderAccessor.CLOSEABLE_RESOURCE, session);
			}
			catch (IOException e) {
				session.close();
				throw new MessageHandlingException(requestMessage,
						"Error handling message in the [" + this
								+ "]. Failed to get the remote file [" + remoteFilePath + "] as a stream", e);
			}
		}

Sample

https://github.com/corentin-pasquier/sample-issue-sftp-connection

You just need a sftp server in local.
You can run this docker command to get one :

docker run -p 22:22 -d atmoz/sftp foo:pass:::upload

Other information

I didn't check if this phenomenom happens with other commands, but it would be interesting to check.

@corentin-pasquier corentin-pasquier added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Jun 24, 2022
@artembilan artembilan added in: file backport 5.5.x and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Jun 24, 2022
@artembilan artembilan added this to the 6.0.0-M4 milestone Jun 24, 2022
@artembilan
Copy link
Member

Confirmed.
That Option.STREAM variant cannot be used for regular this.remoteFileTemplate.execute(session ->) since we must not close the session in the end of the operation, but really defer to that .setHeader(IntegrationMessageHeaderAccessor.CLOSEABLE_RESOURCE, session);.
However, in case error, we really have to close it. Something similar to what you are suggestion.

Feel free to contribute the fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc.

In our tests we use Apache Mina and its SshServer. So, we don't need any external SFTP server for testing.
What you are observing probably can be covered in our SftpServerOutboundTests.

Thank you!

@artembilan artembilan self-assigned this Jul 18, 2022
artembilan added a commit to artembilan/spring-integration that referenced this issue Jul 18, 2022
Fixes spring-projects#3827

The `AbstractRemoteFileOutboundGateway.doGet()` for a `STREAM` option
does not close the `session` in case of error.
This may lead to some leaks or exhausted caches

* Close `session` in the `catch()` of the `AbstractRemoteFileOutboundGateway.doGet()`
* Adjust the `SftpServerOutboundTests` to configure a `CachingSessionFactory`
for the `testStream()` to verify there is no leaks attempting to
`GET STREAM` non-existing remote file twice

**Cherry-pick to `5.5.x`**
garyrussell pushed a commit that referenced this issue Jul 18, 2022
Fixes #3827

The `AbstractRemoteFileOutboundGateway.doGet()` for a `STREAM` option
does not close the `session` in case of error.
This may lead to some leaks or exhausted caches

* Close `session` in the `catch()` of the `AbstractRemoteFileOutboundGateway.doGet()`
* Adjust the `SftpServerOutboundTests` to configure a `CachingSessionFactory`
for the `testStream()` to verify there is no leaks attempting to
`GET STREAM` non-existing remote file twice

**Cherry-pick to `5.5.x`**
garyrussell pushed a commit that referenced this issue Jul 18, 2022
Fixes #3827

The `AbstractRemoteFileOutboundGateway.doGet()` for a `STREAM` option
does not close the `session` in case of error.
This may lead to some leaks or exhausted caches

* Close `session` in the `catch()` of the `AbstractRemoteFileOutboundGateway.doGet()`
* Adjust the `SftpServerOutboundTests` to configure a `CachingSessionFactory`
for the `testStream()` to verify there is no leaks attempting to
`GET STREAM` non-existing remote file twice

**Cherry-pick to `5.5.x`**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants