Skip to content

Fix deadlock in SftpClient.UploadFile upon error #1643

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Rob-Hague
Copy link
Collaborator

When the server returns an error during UploadFile, unless the response is quick enough that the exception is captured before SftpSession.RequestWrite finishes...

if (exception is not null)
{
throw exception;
}

... then the error is not propagated because SftpClient.UploadFile does not have any handling for failure (StatusCode != Ok):

_sftpSession.RequestWrite(handle, offset, buffer, offset: 0, bytesRead, wait: null, s =>
{
if (s.StatusCode == StatusCodes.Ok)
{
_ = Interlocked.Decrement(ref expectedResponses);
_ = responseReceivedWaitHandle.Set();

This causes deadlock because the expectedResponses is only decremented in this path and will never hit zero.

The first commit is the fix for the deadlock (handle failure codes in UploadFile). The second commit tidies up SftpSession.RequestWrite to fix the racy exception behaviour as pointed out in #1640 (the exception handling only makes sense here when given a wait handle).

I wrote an ugly test for this (Rob-Hague@9a7526a) but it only fails on develop after cherry-picking the second commit because the test is single-threaded so we encounter the race in RequestWrite.

closes #957
closes #1640

Copy link
Member

@drieseng drieseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reproduce this using an integration test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SshException check yields a wrong result during normal run, but not while stepping in debugger Deadlock in InternalUploadFile
2 participants