search

eclipse-ecal / fineftp-server

📦 C++ FTP Server Library for Windows 🪟, Linux 🐧 & more 💾
MIT License
311 stars 64 forks source link

Bugfix/delay sending 226 #60

Closed bjuulp closed 12 months ago

bjuulp commented 1 year ago

This pull request is supposed to go on top of the bugfix/pasv_signedness pullrequest. The rationale for the present pull request is that an FTP client implementation (lwftp) has been observed to close the data connection as soon as it receives the 226 status code - even though it hasn't received all data, yet. To improve interoperability with such buggy clients, sending of the 226 status code is delayed a bit.

FlorianReimold commented 1 year ago

Did that issue exist with fineFTP 1.3.5, too?

The thing is that I really hate to introduce artificial delays. The use-case that I initially implemented fineFTP-server for deals with quite a lot of files. And introducing a 100ms delay for every file will hurt performance a lot.

bjuulp commented 1 year ago

I don't know because I have never tested it without the memory mapped files. That is, I think it is likely that this issue has come up due to the improved performance/timing changes with the memory mapped files.

I fully agree that this is an ugly hack that really should be handled by fixing the client side instead. But the thing is that we have a number of clients in the field that have this bug. And to update them to eliminate this bug on the client side, we need them to download over FTP. So it is kind of a chicken-and-egg problem. So we need this workaround on the server side.

Note: We have so far used the Microsoft FTP server and that did not trigger this bug in the client.

I understand if you don't want to merge in this pull request. If so, I'll keep this workaround in our internal builds.

bjuulp commented 1 year ago

Btw. the 100 Ms delay is an async delay. It will not stall any threads. So, it will only downgrade performance of downloads from the same client. It will not impact parallel downloads from different clients.

FlorianReimold commented 1 year ago

Yes, I noticed. So this is the very best implementation of an artificial delay that you can implement.

But it will still block the file transfer of a single client. And 100ms will be very noticeable when transferring a couple of small files. I usually decide in favor of buggy clients and try to support them with workarounds, but in this case the workaround will do too much harm to everybody else.

Is there any other solution for this?

bjuulp commented 1 year ago

I can see if I can find another work around, but I'm not too optimistic.

bjuulp commented 1 year ago

Alternatively, we can make this a configurable feature where the delay is configurable down to 0.

FlorianReimold commented 1 year ago

Hm, what kind of configuration are you thinking about? a compile time config or runtime config?

bjuulp commented 1 year ago

A compile time config. The issue with this is of course that there will be more configurations to test.

FlorianReimold commented 1 year ago

OK, I think I am fine with a compile time option to work around the lwftp bug. However I think that this issue should really be fixed in lwftp. Considering that the bug seems to be known since 3 years, hasn't been fixed and there hasn't been any update on the entire repo for 2 years, that project is probably dead.

bjuulp commented 1 year ago

We've had our issues with the lwftp. Mostly because we use it in a bit different way than originally intended by the author. We contacted the author but he did not seem willing to take in our changes. Since then, we have deviated even more from the original code. So, the best thing to do right now is probably to publish our fork of the lwftp. I'll see if I can get that done soon.

bjuulp commented 1 year ago

I'll also make a new pull request that makes the delay compile time configurable.

florianhumblot commented 7 months ago

@bjuulp have you fine folks made your lwftp version available somewhere? I'm running into the same issue as you describe.

bjuulp commented 7 months ago

Not yet. But we really ought to. I'll try to get this done in the near future. Thanks for reminding me.

bjuulp commented 1 month ago

@florianhumblot @FlorianReimold I've now posted our changes to lwftp here: https://github.com/bjuulp/lwftp/tree/misc_fixes I haven't added any tests, but that should ideally be done at some point. Also, note that the code on that branch comes from a special environment. It hasn't been tested in any other configuration/environment.