spring-integration-sftp: org.apache.sshd.common.io.WritePendingException: A write operation is already pending;

[artembilan]

Discussed in #8707

When we upgraded out application to Spring boot 3 we started getting issues like this:

Caused by: org.apache.sshd.common.io.WritePendingException: A write operation is already pending; cannot write 21 bytes
	at org.apache.sshd.common.channel.ChannelAsyncOutputStream.writeBuffer(ChannelAsyncOutputStream.java:102)
	at org.apache.sshd.sftp.client.impl.DefaultSftpClient.send(DefaultSftpClient.java:299)
	at org.apache.sshd.sftp.client.impl.AbstractSftpClient.checkHandle(AbstractSftpClient.java:230)
	at org.apache.sshd.sftp.client.impl.AbstractSftpClient.openDir(AbstractSftpClient.java:829)
	at org.apache.sshd.sftp.client.impl.SftpDirEntryIterator.<init>(SftpDirEntryIterator.java:60)
	at org.apache.sshd.sftp.client.impl.SftpIterableDirEntry.iterator(SftpIterableDirEntry.java:64)
	at org.apache.sshd.sftp.client.impl.SftpIterableDirEntry.iterator(SftpIterableDirEntry.java:34)
	at java.base/java.lang.Iterable.spliterator(Iterable.java:101)
	at org.springframework.integration.sftp.session.SftpSession.doList(SftpSession.java:103)
	at org.springframework.integration.sftp.session.SftpSession.list(SftpSession.java:69)
	at org.springframework.integration.sftp.session.SftpSession.list(SftpSession.java:52)
	at org.springframework.integration.file.remote.session.CachingSessionFactory$CachedSession.list(CachingSessionFactory.java:227)
	at org.springframework.integration.file.remote.RemoteFileTemplate.lambda$list$5(RemoteFileTemplate.java:422)
	at org.springframework.integration.file.remote.RemoteFileTemplate.execute(RemoteFileTemplate.java:452)
	... 18 common frames omitted

We found out that issues is when you call RemoteFileTemplate.list() method concurrently. Since ChannelAsyncOutputStream.writeBuffer() cannot be caller concurrently.

Before upgrading to SB3 we had no issues like this.

So my question is, is this a bug in new implementation in SB3 using apache mina SSHD or is this expected behaviour and we should not call this method from different threads?

[artembilan]

So, it looks like all the SftpClient operations use in the end this API:

    protected SftpResponse rpc(int cmd, Buffer request) throws IOException {
        int reqId = send(cmd, request);
        return response(cmd, reqId);
    }

where that ChannelAsyncOutputStream.writeBuffer() is done in the send() without any locking monitor.

I believe that:

        IoWriteFuture writeFuture = asyncIn.writeBuffer(buf);
        writeFuture.verify();

should be locked if we cannot call it concurrently. But that's an Apache MINA side.

Since we don't have a choice right now, I'm going to lock all the operations in the SftpSession.
It won't help too much if we use several sessions like in case of CachingSessionFactory, but to be honest we don't need to cache them at all because of async IO nature of SFTP client and this concurrent write buffer limitation.

[baiglin]

Hi @artembilan, I know this one is old, but could you explain why you always use a lock even if you do not use a shared session ? Is there any added value in this case ?

[artembilan]

Hi, @baiglin !

Well, look at the situation like this:
You don't use a isSharedSession, so every time you call DefaultSftpSessionFactory.getSession() you got a fresh session with its own SftpClient.
However there is no guarantee that in the code where you use that session, it is not called concurrently.
So, this lock ensures that custom concurrent usage of the session is guarded.
I don't think there is too much overhead from that lock when the session is not used concurrently.

Or do you want to have some slight optimization where we check for the if (this.sharedSessionLock != null) { before calling that ConcurrentSftpClient.sendLock and leave those possible missuses up the target project?

[baiglin]

Hi again @artembilan ,

Thanks a lot for the quick reply, and no your explanation is enough for me, indeed you cannot expect calling code to use it in a sequential way even if creating a new session each time.

So that answers my question :). I have a similar exception even though I use a fresh client each time, so was wondering the rational behind you fix. But I will open a ticket directly to apache MINA.

By the way if I may ask, I was wondering why; when using a shared session; you now use one session and one client only (which to me is equivalent to a channel) compare to the way it was done with JSch where you were keeping a single session but creating different channels... I think this is due to the async configuration of Apache MINA and fact that it handles path differently but till I wonder?

Thanks again.

[artembilan]

You maybe mean this:

			sftpSession = new SftpSession(sftpClient);
			sftpSession.connect();
			if (this.isSharedSession && freshSftpClient) {
				this.sharedSftpClient = sftpClient;
			}

So, even if we use this.sharedSftpClient, we still create a fresh instance for the SftpSession.
Even if this object is just a wrapper around SftpClient, we still create it.
We indeed might think about making one-to-one between SftpClient and SftpSession even for shared state.
But that's different story, outside of this issue scope.
I don't remember what was there with JSch since I was not involved in development that old solution.

In the end, that's why wrapping into a CachingSessionFactory is always good, so we would not have extra instances for nothing.

[baiglin]

Correct, but from what I check it attempts a reconnect on the channel if not opened, anyway, I wont bother you more if you did not touch the JSch ones ;), thanks for your precious feedbacks.

[artembilan]

@baiglin ,
Please, raise a new issue with description what you think should be improved. Feel free to contribute the fix as well.
It has nothing to do with my knowledge about JSch, rather how reasonable is the current solution and what can be done to make it better.