"readString" not returning everything (FTP)

[Nathipha]

First of all: Thanks for writing a library that works with an FTP server and is easy to use!

I created a new app in Xcode 10 with Swift 5 and the latest version of BlueSocket (installed via CocoaPod) to test it and I've come across a problem:

I can open a normal and a data socket to the FTP server and log in using cmdsocket.write(from: "USER \(username)\r\n") just fine (you can find a list of ftp codes here) but

try cmdSocket.write(from: "LIST\r\n")
let answer = try dataSocket.readString(true)
print(\(answer))

which should list the files and folders at the root only prints the very first one. It's working with a different server (e.g. "speedtest.tele2.net" is free to use for testing), so it looks like "my" server is too slow and/or the library is too fast (and possibly not waiting until the server is done?).

Is there a way to set some kind of timeout for that, e.g. if you don't get a result for the third time, stop reading, instead of stopping after the first try?

[Nathipha]

Found the problem:
BlueSocket stops taking answers if it gets an empty one but with the server being slow and not sending everything at once, this resulted in not receiving all the data.

The solution:
Try again if there's an empty answer.

Code:
Replace the readString() func in Socket.swift with this (then "Clean Build Folder" & "Clean"):

private let maxcount:Int = 10 //max amount of times it'll try receiving an answer
private let maxnilcount:Int = 3 //amount of times it can receive "nil" in a row before it stops

public func readString(_ slow:Bool) throws -> String? {
    if slow { //Use this if you know that the server likes taking its time
        var currlen:Int = 0
        var nilcount:Int = 0
        guard let tempdata = NSMutableData(capacity: 20000) else { //changed to 20000 for long text
            throw Error(code: Socket.SOCKET_ERR_INTERNAL, reason: "Unable to create temporary NSMutableData...2")
        }
            
        for _ in 0..<maxcount {
            if nilcount >= maxnilcount {
                break
            } else {
                let rc = try self.read(into: tempdata)
                                        
                if rc > 0 {
                    currlen = currlen + Int(rc)
                } else {
                    nilcount += 1
                }
            }
                usleep(500) //Wait 500ms for server
        }
            
        guard let str = NSString(bytes: tempdata, length: tempdata.count, encoding: String.Encoding.utf8.rawValue),
        currlen > 0 else {
            throw Error(code: Socket.SOCKET_ERR_INTERNAL, reason: "Unable to convert data to NSString.")
        }
            
        return String(describing: str)
    } else { //Original code
        guard let data = NSMutableData(capacity: 20000) else {                
            throw Error(code: Socket.SOCKET_ERR_INTERNAL, reason: "Unable to create temporary NSMutableData...")
        }
        let rc = try self.read(into: data)
        
        guard let str = NSString(bytes: data.bytes, length: data.length, encoding: String.Encoding.utf8.rawValue),
            rc > 0 else {
                throw Error(code: Socket.SOCKET_ERR_INTERNAL, reason: "Unable to convert data to NSString.")
        }
        return String(describing: str)
    }
}

[billabt]

Consider submitting a pull request with this change... If you do, one required change you'll have to make though is to default slow to false to ensure backwards compatibility with existing users. As it stands right now, it'll probably break somebody since the way you've got it coded, you've changed the signature of the readString() function. The other thing to consider is making the timeout value a parameter as well as the maxcount and maxnilcount_ again, with default values.

The other alternative is to add the functionality as an overloaded version of the current function. You could eliminate the slow parameter and add a timeout parameter and the other counts, for example:

public func readString(maxRetryCount: Int, maxNilCount: Int, retryWaitTime: Int)

Let me know if you have any questions or need help submitting a pull request. Thanks.

[Nathipha]

Sorry, I have no idea how to commit via git.

If someone should come across the same problem, they can just use this code.
You're welcome to add it to the project though - with the changes you suggested. ;)

[Nathipha]

I'm still using the library and it looks like not only reading text is affected, images downloaded with read(into data: inout Data) are also downloaded only partially.

It's not possible not change that function because of the protocol, so I created a new one, using basically the same code as above, except that it appends to Data (which it already did before but not within a loop) and now it's working.

[mbarnach]

@Nathipha Could you make a Pull Request for both issues? It would be an interesting features to have in case of slow connection. Let us know if you need any help to setup the process.

[Nathipha]

@mbarnach
As I said, sorry, I don't know how to commit via git.

[mbarnach]

Would that be OK if I do it for you? I will integrate your changes into a pull request and submit it here.

[Nathipha]

@mbarnach Sure, go ahead. I'm currently not at the mac but I can post the other function (for the data one) here later too.

[mbarnach]

Thanks. Could you have a look at #201 ?

[Nathipha]

Here's my the other edited function:

public func readSlowly(into data: inout Data) throws -> Int {
    let maxcount:Int = 10 //max amount of times it'll try receiving an answer
    let maxnilcount:Int = 3 //amount of times it can receive "nil" in a row before it stops
    var nilcount:Int = 0
        
    // The socket must've been created and must be connected...
    if self.socketfd == Socket.SOCKET_INVALID_DESCRIPTOR {
        throw Error(code: Socket.SOCKET_ERR_BAD_DESCRIPTOR, reason: "Socket has an invalid descriptor")
    }

    if !self.isConnected {
        throw Error(code: Socket.SOCKET_ERR_NOT_CONNECTED, reason: "Socket is not connected")
    }
        
    var returnCount: Int = 0

    for _ in 0..<maxcount {
        if nilcount >= maxnilcount {
            break
        } else {        
            // Read all available bytes...
            let count = try self.readDataIntoStorage()

            // Did we get data?
            if count > 0 {
                // - Yes, move to caller's buffer...
                data.append(self.readStorage.bytes.assumingMemoryBound(to: UInt8.self), count: self.readStorage.length)
                returnCount = returnCount + self.readStorage.length
                // - Reset the storage buffer...
                self.readStorage.length = 0
            } else {
                nilcount += 1
            }
        }
            
        usleep(500) //500ms
    }
    return returnCount
}

I was just testing it with 9mb images but the first call never returns more than 430kb, so looks like the server I'm testing it with is especially slow - or maybe my internet is too slow and it stops receiving more data after a certain amount of time (is that a thing?). I was only able to get all the data by looping over the function until it retuns 0 bytes (using the default "stream" mode, so it closes the data socket and from then on only returns 0 bytes), which doesn't necessarily look like the best solution.

Your commit looks pretty similar and it'll probably give the same result unless I set retry to some stupidly high number. Unfortunately I won't be able to work on this again until next week (after the easter holidays) but I'll get back to you once I find a satisfying solution.

[dannys42]

Hi @Nathipha thanks for the detailed description of the problem. And thanks @mbarnach for expanding and submitting a PR. I added more detailed comments there, but just for continuity, I'll put my suggestion here as well.

The short of it is that you're not guaranteed of data length in socket calls. So I believe your FTP application should do something like this:

• have a loop that contains a Socket.wait(), and inside the wait:
• do a readString()
• append the result to a buffer
• scan the buffer for newlines.
• If a newline exists, pull out the command and process it, then remove everything up to the newline from the buffer (leaving the rest there), then continue the loop
  ( If a newline does not exist, do nothing and continue the loop

It's a little bit of work, but it's typical of what I've had to do when working with sockets at a low level.

This should work in the case of blocking or non-blocking sockets. If you're attempting to write an FTP server that can support multiple clients, then you'll definitely want to go the non-blocking route.

[Nathipha]

@dannys42
Did I understand your suggestion correctly? Leave both "read" functions the way they were originally and do that stuff in my own code instead, so something like:

while true {
    var data2 = Data()
    let bytesRead2 = try self.dataSocket.readSlowly(into: &data2)

    if bytesRead2>0 {
        data.append(data2)
        //TODO: wait
    } else {
        break
    }                              
}

This is what I added as a quick fix before I left a week ago because even though downloading data was more successful with the changes I made to the library itself, it still doesn't come close to finishing every time (as mentioned in my last post) and I'll be honest, I don't know what's the reason for that, if it's the timeout, the server, my internet connection, or simply the way FTP works.
Just this short code won't work without the changes I made though because the server sends 0 bytes too often, even when there's still data left. The only two ways (I can think of) to see if it's done is to either compare the file sizes (on the server vs. downloaded data) or to check for remoteConnectionClosed. The second one might be a problem if the connection is closed too early (is that possible if there's a stable connection?), so not sure what I'll go for yet but I'm back to working on it now.

[dannys42]

The general idea is correct, but if you're trying to make a TCP server that can listen and respond to multiple clients, that isn't usually the way you structure it. I'll see if I can work out an example, but it might take me a while before I can get to it. Just curious, have you worked with TCP sockets in other languages before?

[Nathipha]

@dannys42
I made the same app for Java too, both using a library (by Apache) and working with Java's Socket direktly. The overall approach is the same of course because you're sending/receiving the same commands to/from the FTP server.
To be honest, I'm not trying to fix everything in BlueSocket. It's currently the only library that works (and wasn't last updated 5+ years ago) and I just want to make it work with my current FTP server.

Edit: I changed readSlowly:
Instead of for _ in 0..<maxcount I'm now using while !remoteConnectionClosed and also removed while true in the class that's handling the sockets. maxnilcount is still used but as soon as 0 bytes are returned once (= socket closed), it breaks the loop automatically. This works, so looks like the FTP server I'm using is never actually returning 0 bytes but instead only returning really small pieces.
I have to check if SIZE is supported by the FTP server but if it is, then I might also add a check to see if it downloaded all the bytes (and didn't simply lose connection).

[dannys42]

Thanks @Nathipha, I can sympathize with wanting to get your FTP server done and not having to dig too far into BlueSocket. For what it's worth, this is pretty typical behavior for low-level sockets. Reading your edit, I believe what you're doing now in your readSlowly call is correct.

I managed to write some tests over the weekend to confirm whether or not there are any issues in BlueSocket and more thoroughly stress test it in a streaming situation. I did not find any technical issues. However in writing the tests I did have to add Hashable and Equatable conformance to make connection state management a bit easier.

I plan to do a bit more refactoring and adding some more documentation to make the logic easier to follow. But it might take me a few days to get to it. I pushed what I have so far into an open PR in case it helps you.