ArxOne / FTP

Simple FTP client
MIT License
37 stars 15 forks source link

FtpStream.Release will be blocking after Filezilla replied 550 #17

Closed Onway closed 8 years ago

Onway commented 8 years ago
    [TestMethod]
    [TestCategory("Ftp")]
    [TestCategory("RequireHost")]
    public void FtpListNoDirectory()
    {
        var ftpTestHost = GetTestHost("ftp");
        using (var ftpClient = new FtpClient(ftpTestHost.Uri, ftpTestHost.Credential))
        {
            var list = ftpClient.ListEntries("/DirectoryNotFound/");
        }
    }

After replied 550, the server would not give the second one, then FtpStream.Release(true) will be blocking.

    protected void Release(bool expectEndReply)
    {
        var session = Session;
        Session = null;
        if (session != null)
        {
            try
            {
                if (expectEndReply)
                {
                    Process(() => session.Expect(
                        226, // default ack
                        150 // if the stream was opened but nothing was sent, then we still shall exit gracefully
                        ));
                }
            }
            // on long transfers, the command socket may be closed
            // however we need to signal it to client
            finally
            {
                session.Release();
            }
        }
    }
Onway commented 8 years ago

If I simply add a 550 in var reply = Expect(SendCommand(handle, "LIST", EscapePath(path.ToString())), 125, 150, 425); I received a NullReferenceException in using (var streamReader = new StreamReader(dataStream, ((IFtpStream)dataStream).ProtocolEncoding)) since dataStream has aborted.

More generally, should we Expect unsuccessful reply code? Would it be better if directly throw to user like this:

    // class FtpSession
    public static FtpReply Expect(FtpReply reply, params int[] codes)
    {
        if (!codes.Any(code => code == reply.Code))
        {
            if (reply.Code.Class == FtpReplyCodeClass.Connections)
                throw new IOException();
            else
                throw new UnexcpetedReplyException(reply);
        }
        return reply;
    }

    // in some FtpClient methods
    using (var dataStream = OpenDataStream(handle, FtpTransferMode.Binary))
    {
        try
        {
            Expect(SendCommand(handle, "LIST", EscapePath(path.ToString())), 125, 150);
        }
        catch
        {
            Abort(dataStream);
            throw;
        }

        // read from dataStream
    }

:grin:

picrap commented 8 years ago

Thanks for suggestion, I'll take a look at it soon.

picrap commented 8 years ago

Fixed in version 1.6