Skip to content
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

SFTP file read very slow, length is ignored #68

Open
deze333 opened this issue Aug 22, 2024 · 3 comments
Open

SFTP file read very slow, length is ignored #68

deze333 opened this issue Aug 22, 2024 · 3 comments
Assignees

Comments

@deze333
Copy link

deze333 commented Aug 22, 2024

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:

  • OS: macOS Sonoma
  • Client: Citadel

Server:

  • OS: linux
  • Server: remote SFTPD

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
Copy link
Member

Joannis commented Aug 22, 2024

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
Copy link
Author

deze333 commented Aug 23, 2024

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
Copy link
Member

Joannis commented Sep 4, 2024

I'll look into it after my vacation.

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

No branches or pull requests

2 participants