SFTP parallel downloads with one connection - Exception (not threadsafe?)

Jan 14, 2013 at 8:19 AM
Edited Jan 14, 2013 at 10:06 AM

Hi!

Tried the following:

- open one SFTP-Connection to a server

- starting several downloads parallel (tried sync and async)

- Exception is thrown by checking if "isConnected" (it's called before starting the download)

 

Because this Exception is not thrown every time, there might be a problem related to

thread safetiness. Anyone else discovered this problem?

 

I know that it is possible to open several connections parallel but this takes time.

So is there a chance to fix it or am I doing something wrong?

 

 

 

Coordinator
Jan 14, 2013 at 7:58 PM

Hi,

 

Can you post code example that you use t download?

Also, what kind of exception does it throw?

 

Thanks,

Oleg

 

Jan 15, 2013 at 8:18 AM
Edited Jan 15, 2013 at 8:34 AM

 

Hi,

the exception shows up random and is hard to provocate.
EnsureConnection seems to be the problem.


Exception:

Renci.SshNet.Common.SshConnectionException: Client not connected.
   bei Renci.SshNet.BaseClient.EnsureConnection()
   bei Renci.SshNet.SftpClient.BeginDownloadFile(String path, Stream output, AsyncCallback asyncCallback, Object state, Action`1 downloadCallback)
   bei ConsoleApplication1.Module1.Main()


Code:
 
            oSFTPClient = New Renci.SshNet.SftpClient("127.0.0.1", "user", "pass")
            oSFTPClient.Connect()

            oQ = New Queue(Of IO.Stream)()

            For i As Integer = 1 To iCount
                oQ.Enqueue(New IO.FileStream("c:\test\test" & i & ".dat", IO.FileMode.CreateNew))
            Next

            If oSFTPClient.IsConnected Then
                For i As Integer = 1 To iCount
                    Dim oStream As IO.Stream = oQ.Dequeue()
                    oSFTPClient.BeginDownloadFile("d:\file.dat", oStream, AddressOf DownloadDone, Nothing)
                Next
            End If

        Catch ex As Exception
            Debug.WriteLine(ex.ToString)
        End Try

Coordinator
Jan 15, 2013 at 2:52 PM

Hi,

 

I was actually trying to fix that problem recently so I could cover most cases and I guess I am still missing something.

 

Can you put a breakpoint in partial void IsSocketConnected(ref bool isConnected) method to see what condition causes isConnected to be false.

You can put a conditional breakpoint, so it will stop only when isConnected is false, then you can try to see which one is actually false.

I suspect that connection is actually still connected but one of the condition probably false for whatever reason which is causes everything to fail.

 

Thanks,

Oleg

Jan 15, 2013 at 7:16 PM
Edited Jan 15, 2013 at 7:18 PM

I took the time to investigate the problem a bit further.

The problem is here:

 

        partial void IsSocketConnected(ref bool isConnected)
        {

            // isConnected = (!this._isDisconnecting && this._socket != null && this._socket.Connected && this._isAuthenticated && this._messageListenerCompleted != null) && !(this._socket.Poll(10, SelectMode.SelectRead));
            isConnected = (!this._isDisconnecting && this._socket != null && this._socket.Connected && this._isAuthenticated && this._messageListenerCompleted != null)
               && !(this._socket.Poll(1, SelectMode.SelectRead) && this._socket.Available == 0);

        }

 

 

Found out that:

!(this._socket.Poll(1, SelectMode.SelectRead) && this._socket.Available == 0)

returns a wrong value. Don't know why, but do you really need to check this?

You already checked if socket.Connected ...

 

 

Thanks!

Jan 15, 2013 at 7:34 PM
Edited Jan 15, 2013 at 7:37 PM

ok tested a bit more.

 

If everything is fine then:

this._socket.Poll(1, SelectMode.SelectRead) -> = true
this._socket.Available == 0 -> = false

 

But when the exception is thrown (because of massive parallel calling of IsSocketConnected) then

this._socket.Poll(1, SelectMode.SelectRead) -> = true (OK!)
this._socket.Available == 0 -> = true (NOT OK!)


Hope that helps!

Coordinator
Jan 15, 2013 at 8:47 PM

Hi,

 

Thanks for testing.

First, wanted to explain a little, IsConnect property is not always correct, since it will not handle situation where connection was dropped by the server in this case IsConnect still will be true until next network request, so in order to truly determine if socket is connected you need to for a little more.

Now, you actually made me thing of another possible reason for that.  I am locking all socket related operation except this one and I think it might causing the problem.

 

Can you try this:

        partial void IsSocketConnected(ref bool isConnected)
        {
            lock (this._socketLock)
            {
                isConnected = (!this._isDisconnecting && this._socket != null && this._socket.Connected && this._isAuthenticated && this._messageListenerCompleted != null)
                    && !(this._socket.Poll(1, SelectMode.SelectRead) && this._socket.Available == 0);
            }
        }

And let me know if you still experience the problem.

 

Thanks,

Oleg

Jan 15, 2013 at 9:31 PM
Edited Jan 15, 2013 at 10:02 PM

Thanks for the fast reply.

This was the first thing i thought off and so i tried the same as you described but that doesn't help.

There must be another reason for this failure to occur (it's not server related - tried different ones).

 

Edit: I also tried to force "isConnected = true" even when (this._socket.Available == 0)  -> = true.

And the download went perfectly fine. So (this._socket.Available == 0) reports a wrong value. Maybe a bug?

I also found this way (which you actually use) to truly determine if the socket is still connected as example in different forums. Can't imagine that i'am the first one who encounters a problem with this kind of test. Confusing :-).

Coordinator
Jan 15, 2013 at 11:09 PM

hmm,

It becomes more difficult now (

I will try to investigate a little more then it but wont have time for it in near future so if you happened to find a solution please let me know.

I guess another idea just try to ignore (this._socket.Available == 0) test then. Its just everywhere I checked, everybody recommend to use those to tests together

Thanks,

Oleg

Coordinator
Jan 17, 2013 at 1:47 AM

ok,

 

It seems that correct way would be to remove this._socket.Available == 0 test.

Since it's completly legal to either have data or not at a time of checking socket.

So can you remove that test and see if it works and let me know.

If it works, then I guess I will remove that test from there.

 

Thanks,

Oleg

Jan 17, 2013 at 8:03 AM

But that would make testing of < this._socket.Poll(1, SelectMode.SelectRead) > useless, because it almost always returns <true>.

See MSDN for that: http://msdn.microsoft.com/de-de/library/system.net.sockets.socket.poll.aspx

 

Coordinator
Jan 17, 2013 at 11:53 AM
Edited Jan 17, 2013 at 11:55 AM

Hmm, yea, true, you right.

I guess this method is not as intuitive is its name says,

I will look into may be using combination of pooling for read with polling for write.

Or actually now when  I think about it, may be it needs to be pooling for write, which will return true when data can be sent, and thats where I check for IsConnected property anyway, before sending any command, basically writing to the socket.

 

I will play with that, to see how it works, but I guess if you like can you try that too, using only polling method but for write. So try it like that:

 

        partial void IsSocketConnected(ref bool isConnected)
        {
                isConnected = (!this._isDisconnecting && this._socket != null && this._socket.Connected && this._isAuthenticated && this._messageListenerCompleted != null)
                    && this._socket.Poll(-1, SelectMode.SelectWrite);
        }

 

I will read more about it later today but this is a first thing that came to my mind as I woke up :):).

 

Thanks,

Oleg

Jan 18, 2013 at 4:47 PM
Edited Jan 18, 2013 at 4:47 PM

Hi,

that seems to work. Thanks!

Just another short question: When I start a download (with BeginDownloadFile) and the download went fine the asyncCallback Method is called. So I know that everything is done.

But what if the Download fails, AFTER it was started (and the start went through without throwing an exception). An example: Starting a download of a very big file … letting it download some megabytes and then force a disconnect. So where do I get the info that this file failed to download completely? There is no exception thrown, whether the <ErrorOccured> event is raised.

 

Thanks again!

Coordinator
Jan 18, 2013 at 4:52 PM

Hey,

From as far as I remember,by design it was that when you call EndDownload method you should get an exception.

So ideally should work like that.

You BeginDownload

Then do something and when you get notified that it was finished, successfully or not, you call EndDownload and thats where you should get an exception.

You should always call End*** operation on any Begin**, this is what Microsoft recommends at least, so I was trying to follow there guidelines.

 

Hope it helps,

Thanks,

Oleg

Jan 18, 2013 at 5:02 PM

Yeah I'm dumb ... silly question (forgot to check the asyncCallback Method, because I thought it will only be triggered when the Download succeeded!)

So thanks for your time and help, you’re doing a great job here!

Will you commit the changes to the IsConnected check in near future?


Coordinator
Jan 18, 2013 at 5:20 PM

no problem,

 

Yea, I just did. Thanks for testing it for me.

 

Thanks,

Oleg