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

Deadlock when using SFTP CachingSessionFactory with FileExistsMode.FAIL #3249

Closed
javierestevez opened this issue Apr 17, 2020 · 3 comments
Closed

Comments

@javierestevez
Copy link

Affects 5.2.5

It looks like there is a deadlock when using a CachingSessionFactory and FileExistsMode.FAIL. When FileExistsMode.FAIL is used, then we first check if the file exists in the server by calling RemoteFileTemplate#exists which in turn calls RemoteFileTemplate#execute. The latter requests a new session to the session factory instead of using the session already obtained (RemoteFileTemplate#activeTemplateCallbacks size always evaluated to 0 during my tests). This can end up in a deadlock situation if there are as many (or less) cached sessions as the number of concurrent threads (possibly with more too).

Below is a test I used to reproduce this locally using 2 concurrent threads and 2 cached sessions. I omitted some details for brevity. Hopefully it is enough to reproduce it on your side.

private final Runnable runnable = () -> {
    UUID uuid = UUID.randomUUID();
    String filename = uuid + ".tst";
    LOGGER.info("Uploading file " + filename + " to SFTP...");

    testSftpGateway.send(DIRECTORY_NAME, filename, "Some bytes".getBytes());
    LOGGER.info("... Uploaded " + filename + "!");
};

@Test
public void test() {
    withConfiguredSftpServer(sftpServer -> {
        ExecutorService executorService = Executors.newFixedThreadPool(2);
        for (int i = 0; i < 100; i++) {
            executorService.execute(runnable);
        }
        executorService.shutdown();
        executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
        LOGGER.info("Finished all threads");
    });
}
@MessagingGateway
interface TestSftpGateway {
    @Gateway(requestChannel = OUTBOUND_CHANEL)
    void send(@Header(FileHeaders.REMOTE_DIRECTORY) String directoryName,
              @Header(FileHeaders.FILENAME) String fileName,
              @Payload byte[] payload);
}

@Configuration
@IntegrationComponentScan
@EnableIntegration
static class TestConfiguration {

    @Bean
    public SessionFactory<ChannelSftp.LsEntry> sftpSessionFactory() {
        DefaultSftpSessionFactory factory = new DefaultSftpSessionFactory(true);
        factory.setHost("localhost");
        factory.setPort(22);
        factory.setUser("foo");
        factory.setPassword("pass");
        factory.setAllowUnknownKeys(true);

        CachingSessionFactory<ChannelSftp.LsEntry> cachingSessionFactory = new CachingSessionFactory<>(factory, 2);
        cachingSessionFactory.setTestSession(true);
        cachingSessionFactory.setSessionWaitTimeout(1000);

        return cachingSessionFactory;
    }

    @Bean
    public SftpRemoteFileTemplate sftpRemoteFileTemplate(SessionFactory<ChannelSftp.LsEntry> sftpSessionFactory) {
        FunctionExpression<Message<?>> remoteDirectoryExpression = new FunctionExpression<>(this::getRemoteDirectoryHeaderValue);
        SftpRemoteFileTemplate sftpRemoteFileTemplate = new SftpRemoteFileTemplate(sftpSessionFactory);
        sftpRemoteFileTemplate.setRemoteDirectoryExpression(remoteDirectoryExpression);
        return sftpRemoteFileTemplate;
    }

    @Bean
    public IntegrationFlow outboundFlow(SftpRemoteFileTemplate template) {
        return IntegrationFlows.from(OUTBOUND_CHANEL)
                .handle(Sftp.outboundAdapter(template, FileExistsMode.FAIL)
                        .useTemporaryFileName(false)
                        .remoteDirectory(this::getRemoteDirectoryHeaderValue))
                .get();
    }

    private String getRemoteDirectoryHeaderValue(Message<?> message) {
        MessageHeaders messageHeaders = message.getHeaders();
        return (String) messageHeaders.get(FileHeaders.REMOTE_DIRECTORY);
    }
}
@artembilan
Copy link
Member

The RemoteFileTemplate.doSend() has to be fixed:

	private void doSend(Session<F> session, FileExistsMode mode, String remoteFilePath, String tempFilePath,
			InputStream stream) throws IOException {

	...
		else {
			if (exists(remoteFilePath)) {
		...
			}
		}
	}

We should not call that exists(remoteFilePath) on the template, but rather a provided session.exists(remoteFilePath).

Good catch!

I'll try to fix it over weekend.
Thank you for the code to let us to reproduce!

@artembilan artembilan added this to the 5.3 RC1 milestone Apr 17, 2020
artembilan added a commit to artembilan/spring-integration that referenced this issue Apr 19, 2020
Fixes: spring-projects#3249

When the `CachingSessionFactory` is configured with small enough pool
and it is very likely that dead lock may happen when `RemoteFileTemplate.send()`
is used.
The problem happens when we reach the `RemoteFileTemplate.exists()` call
which is done from the internal method called from already pulled from cache
`Session`

* Fix `RemoteFileTemplate` to use a `session.exists()` instead on the provided
into the method `Session`
* Demonstrate the problem in the `SftpRemoteFileTemplateTests.testNoDeadLockOnSend()`

**Cherry-pick to 5.2.x, 5.1.x & 4.3.x**
@artembilan
Copy link
Member

Reopen: merged to the master accidentally: ... 😢

See linked PR for 5.2.x branch.

@artembilan artembilan reopened this Apr 19, 2020
garyrussell pushed a commit that referenced this issue Apr 20, 2020
Fixes: #3249

When the `CachingSessionFactory` is configured with small enough pool
and it is very likely that dead lock may happen when `RemoteFileTemplate.send()`
is used.
The problem happens when we reach the `RemoteFileTemplate.exists()` call
which is done from the internal method called from already pulled from cache
`Session`

* Fix `RemoteFileTemplate` to use a `session.exists()` instead on the provided
into the method `Session`
* Demonstrate the problem in the `SftpRemoteFileTemplateTests.testNoDeadLockOnSend()`

**Cherry-pick to 5.2.x, 5.1.x & 4.3.x**

Fix RemoteFileOutboundGWTests for the proper mock
garyrussell pushed a commit that referenced this issue Apr 20, 2020
Fixes: #3249

When the `CachingSessionFactory` is configured with small enough pool
and it is very likely that dead lock may happen when `RemoteFileTemplate.send()`
is used.
The problem happens when we reach the `RemoteFileTemplate.exists()` call
which is done from the internal method called from already pulled from cache
`Session`

* Fix `RemoteFileTemplate` to use a `session.exists()` instead on the provided
into the method `Session`
* Demonstrate the problem in the `SftpRemoteFileTemplateTests.testNoDeadLockOnSend()`

**Cherry-pick to 5.2.x, 5.1.x & 4.3.x**

Fix RemoteFileOutboundGWTests for the proper mock
@artembilan
Copy link
Member

Fixed with the merged PR.

garyrussell pushed a commit that referenced this issue Apr 20, 2020
Fixes: #3249

When the `CachingSessionFactory` is configured with small enough pool
and it is very likely that dead lock may happen when `RemoteFileTemplate.send()`
is used.
The problem happens when we reach the `RemoteFileTemplate.exists()` call
which is done from the internal method called from already pulled from cache
`Session`

* Fix `RemoteFileTemplate` to use a `session.exists()` instead on the provided
into the method `Session`
* Demonstrate the problem in the `SftpRemoteFileTemplateTests.testNoDeadLockOnSend()`

**Cherry-pick to 5.2.x, 5.1.x & 4.3.x**

Fix RemoteFileOutboundGWTests for the proper mock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants