search

orlandos-nl / Citadel

SSH Client & Server in Swift
MIT License
219 stars 35 forks source link

SFTP file read very slow, length is ignored #68

Open deze333 opened 3 weeks ago

deze333 commented 3 weeks ago

Even though SFTPFile methods read(from offset:length:) and readAll() request specific length of bytes to be read, the underlying NIO channel ignores that and seems to discard all data past its default chunk size value which is effectively:

NonBlockingFileIO.defaultChunkSize = 128 * 1024  // 131072 bytes or 128kb

This results in very slow download times as the same portions of file are being multiple times but NIO stripping (or so it seems) all data past 131072 bytes for each request. Effectively the file is being read many times over again and again.

The only way to somewhat remedy the problem is to use the length of 131072 but the download speed is still too slow due to many requests needed with 128k chunks.

The file's attributes are known and its attributes.size contains correct number of bytes. The readAll() attempts to read the file at once using its attributes.size value.

Code to reproduce:

let buffer = try await file.readAll()

Client:

Server:

Additional context

The log of reading a 5Mb file where readAll() is requesting full file download but length is being ignored and replaced by default NIO value.

[ DEBUG ] SFTP starting chunked read operation on file 0100 (Citadel/SFTPFile.swift:102)
[ TRACE ] SFTP OUT: read{7}(0100, 5207962 bytes from 0) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{8}(0100, 5076890 bytes from 131072) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{9}(0100, 4945818 bytes from 262144) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{10}(0100, 4814746 bytes from 393216) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{11}(0100, 4683674 bytes from 524288) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
...
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{42}(0100, 620442 bytes from 4587520) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{43}(0100, 489370 bytes from 4718592) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{44}(0100, 358298 bytes from 4849664) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{45}(0100, 227226 bytes from 4980736) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 131072 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ TRACE ] SFTP OUT: read{46}(0100, 96154 bytes from 5111808) (Citadel/SFTPClient.swift:93)
[ DEBUG ] SFTP read 96154 bytes from file 0100 (Citadel/SFTPFile.swift:81)
[ DEBUG ] SFTP completed chunked read operation on file 0100 (Citadel/SFTPFile.swift:128)
Joannis commented 3 weeks ago

Hey @deze333 , sorry to say but the packet size limit you're running into is not a bug in Citadel nor SwiftNIO. The variable you linked is not used by Citadel or NIOSSH, but is a separate variable that references the same constant. It's maybe not a coincidence that both use the same value, as it's a more commonly used value for limiting message sizes in network protocols.

The issue you're running into is caused by your SFTP server, likely openssh, which specifies a 1 mebibyte limit for any packet. The read limit is a result of that, and cannot be increased by a client such as Citadel.

That being said, I think what we can do is look at parallelising this work - if the SFTP protocol allows for that.

deze333 commented 3 weeks ago

Thank you @Joannis for casting light on this problem. Looks like it's the openssh limitation. I have compared download speed with both lftp and sftp and they don't seem to have that problem perhaps because they parallelise the download process, just as you suggested. The SFTP server certainly allows that.

Now I imagine it would be a non-trivial work to extend Citadel to support download parallelism. That would make a huge sense though. Fingers crossed!

Joannis commented 1 week ago

I'll look into it after my vacation.