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.

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

Jump to bottom

RemoteFileTemplate needlessly marks session dirty in case of no such file error #8745

Closed
kdebski85 opened this issue Sep 29, 2023 · 2 comments · Fixed by #8759
Closed

RemoteFileTemplate needlessly marks session dirty in case of no such file error #8745

kdebski85 opened this issue Sep 29, 2023 · 2 comments · Fixed by #8759
Assignees
@artembilan
Labels
(EOL) for: backport-to-6.1.x in: file in: ftp in: sftp in: smb type: bug
Milestone
6.2.0-RC1

Comments

@kdebski85
Copy link
Contributor

kdebski85 commented Sep 29, 2023 •
edited

In what version(s) of Spring Integration are you seeing this issue?

6.1.3

Describe the bug

The issue applies to all classes which extend org.springframework.integration.file.remote.RemoteFileTemplate. The description below uses SftpRemoteFileTemplate from spring-integration-sftp as an example.

RemoteFileTemplate::execute method marks session dirty in case of any exception.
It prevents session to be reused and makes CachingSessionFactory inefficient.
As a result, too many sessions are created and SFTP server may reject new connections.
Certain exceptions, like SftpConstants.SSH_FX_NO_SUCH_FILE , should not cause session to be marked dirty.

To Reproduce

  1. Configure DefaultSftpSessionFactory to point to some valid SFTP
  2. Wrap the factory with CachingSessionFactory
  3. Configure SftpRemoteFileTemplate to use created CachingSessionFactory
  4. Use SftpRemoteFileTemplate::list method with some path that does not exist on the server

Expected behavior

  1. SftpRemoteFileTemplate::list throws exception (the current behaviour)
  2. Session is not marked dirty and can be reused by further calls (currently it is marked dirty and cannot be reused)

Probably each class that extends RemoteFileTemplate should be able to define strategy for differentiating if an exception should mark session dirty.

Workaround
Use SftpRemoteFileTemplate::exists before each SftpRemoteFileTemplate::list (call list only when exists returns true)
@kdebski85 kdebski85 added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Sep 29, 2023
@artembilan
Copy link
Member

I think we can come up with something like protected boolean shouldMarkSessionAsDirty(Exception ex) with some default implementations for those RemoteFileTemplate extensions (default one should just return true).
If there is some peculiar situation where for some exception you must not mark it as dirty, then you will be able to extend SftpRemoteFileTemplate and override this method.

Let us know if you are will to contribute the fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc

Thank you!

@artembilan artembilan added this to the 6.2.0-RC1 milestone Oct 3, 2023
@artembilan artembilan self-assigned this Oct 9, 2023
artembilan added a commit to artembilan/spring-integration that referenced this issue Oct 10, 2023
@artembilan
spring-projectsGH-8745: Add RFT.shouldMarkSessionAsDirty()
2cb41b1 
Fixes spring-projects#8745

Not all errors caught in the `RemoteFileTemplate.execute()`
are fatal to mark session as dirty and physically close the target session
in the cache

* Introduce a `RemoteFileTemplate.shouldMarkSessionAsDirty()`
to consult with an exception if it is really a fatal error to close
the session in the end.
* Override `shouldMarkSessionAsDirty()` in the `RemoteFileTemplate`
implementations to check statuses of respective protocol errors

**Cherry-pick to `6.1.x` & `6.0.x`**
@artembilan artembilan mentioned this issue Oct 10, 2023
Merged
garyrussell pushed a commit that referenced this issue Oct 11, 2023
@artembilan
GH-8745: Add RFT.shouldMarkSessionAsDirty() (#8759)
44433ed 
* GH-8745: Add RFT.shouldMarkSessionAsDirty()

Fixes #8745

Not all errors caught in the `RemoteFileTemplate.execute()`
are fatal to mark session as dirty and physically close the target session
in the cache

* Introduce a `RemoteFileTemplate.shouldMarkSessionAsDirty()`
to consult with an exception if it is really a fatal error to close
the session in the end.
* Override `shouldMarkSessionAsDirty()` in the `RemoteFileTemplate`
implementations to check statuses of respective protocol errors

**Cherry-pick to `6.1.x` & `6.0.x`**

* * Fix tests for pool interaction

* * Fix language in Javadocs
* Add more `not dirty` statuses to `SftpRemoteFileTemplate` & `SmbRemoteFileTemplate`
garyrussell pushed a commit that referenced this issue Oct 11, 2023
@artembilan @garyrussell
GH-8745: Add RFT.shouldMarkSessionAsDirty() (#8759)
274a3e0 
* GH-8745: Add RFT.shouldMarkSessionAsDirty()

Fixes #8745

Not all errors caught in the `RemoteFileTemplate.execute()`
are fatal to mark session as dirty and physically close the target session
in the cache

* Introduce a `RemoteFileTemplate.shouldMarkSessionAsDirty()`
to consult with an exception if it is really a fatal error to close
the session in the end.
* Override `shouldMarkSessionAsDirty()` in the `RemoteFileTemplate`
implementations to check statuses of respective protocol errors

**Cherry-pick to `6.1.x` & `6.0.x`**

* * Fix tests for pool interaction

* * Fix language in Javadocs
* Add more `not dirty` statuses to `SftpRemoteFileTemplate` & `SmbRemoteFileTemplate`
garyrussell pushed a commit that referenced this issue Oct 11, 2023
@artembilan @garyrussell
GH-8745: Add RFT.shouldMarkSessionAsDirty() (#8759)
7d73e52 
* GH-8745: Add RFT.shouldMarkSessionAsDirty()

Fixes #8745

Not all errors caught in the `RemoteFileTemplate.execute()`
are fatal to mark session as dirty and physically close the target session
in the cache

* Introduce a `RemoteFileTemplate.shouldMarkSessionAsDirty()`
to consult with an exception if it is really a fatal error to close
the session in the end.
* Override `shouldMarkSessionAsDirty()` in the `RemoteFileTemplate`
implementations to check statuses of respective protocol errors

**Cherry-pick to `6.1.x` & `6.0.x`**

* * Fix tests for pool interaction

* * Fix language in Javadocs
* Add more `not dirty` statuses to `SftpRemoteFileTemplate` & `SmbRemoteFileTemplate`
@kdebski85
Copy link
Contributor Author

@pakorcz thank you for finding the source of the issue!
@artembilan thank you for fixing the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@artembilan artembilan

Labels
(EOL) for: backport-to-6.1.x in: file in: ftp in: sftp in: smb type: bug
Projects
None yet
Milestone
6.2.0-RC1
Development

Successfully merging a pull request may close this issue.

GH-8745: Add RFT.shouldMarkSessionAsDirty() artembilan/spring-integration
2 participants
@artembilan @kdebski85