Upload large file getting stuck

[mironabr]

I was using the original jsch lib and had the issue that uploading large file getting stack and I applied the following fix to the code and it solved the issue.
I want to move to your lib but you must apply this fix as well:
is/jsch#39

[norrisjeremy]

Hi @mironabr,

Please see #54 that the original author of is/jsch#39 opened here.
The fix proposed didn't seem to fully resolve the issue based upon my testing at the time.
I'm also extremely hesitant to merge the proposed fix as I'm not convinced it wouldn't introduce regressions for some folks.

Thanks,
Jeremy

[mironabr]

We moved from the original jsch to this lib. It works. However, our QA team made performance tests and said that uploading file to SFTP server with the original lib is much faster. Do you know something about it ?

[mironabr]

My QA team did lot of tests with the original jsch lib and mwiede lib and they 100% confirmed that uploading large file (20MB and above), most of the times both libs are getting stuck but when I added the above fix (is/jsch#39) both libs worked and were not stuck.
Also the QA team confirmed that uploading using the original jsch lib is much faster

[norrisjeremy]

Hi @mironabr,

I don't know immediately why your team would have found that this fork is slower than the older version of than this version, since you haven't provided any data that can be analyzed to help determine why.
If I had to guess, my initial suspicion would because our version may using different crypto algorithms than the original with your server, since we have added several more modern and secure crypto algorithms into this fork, that likely have different performance characteristics.

As for the upload issue, I'm not especially comfortable with merging the fix, since I was still able to trigger a deadlock even with it applied, and the original author of it has remained silent and never answered questions I posed to help better understand the proposed fix.

Thanks,
Jeremy

[norrisjeremy]

@mwiede What are your thoughts? Do you think we should simply merge the change from is/jsch#39 here? I'm not sure how much of a regression risk there would be for it, but if users are reporting it helps solve their problems, perhaps it's worth a gamble.

[norrisjeremy]

@mwiede What are your thoughts? Do you think we should simply merge the change from is/jsch#39 here? I'm not sure how much of a regression risk there would be for it, but if users are reporting it helps solve their problems, perhaps it's worth a gamble.

Or perhaps we add some configuration setting that controls whether the additional flush() call is executed, so that there is at least a way for users to opt-in (if it helps their use cases) or opt-out (in case it causes a regression of some sort)?

[mironabr]

Hi @norrisjeremy & @mwiede ,
I guess you are right about the encryption and performance. I added the following line to my code:
config.put("server_host_key", "ssh-ed25519,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,rsa-sha2-512,rsa-sha2-256,ssh-rsa,ssh-dss");
because I have a client that uses an old sftp server and after that the performance was the same as the original lib.
So your lib with older servers has the same performance as the original.

About the upload getting stack, I don't know when did you check the mention fix suggestion (is/jsch#39) but we made lot of tests. mainly on the original lib. To fix the upload stack I had to add both fixes to the code:
is/jsch#39 +
https://sourceforge.net/p/jsch/mailman/message/36872566/

The second fix was already implemented in your lib. If you will add the first fix (with the flush) you will see it will solve the issue. More than that - our QA team reported that with the flush fix, the performance was improved

Regards
Miron

[norrisjeremy]

Hi @mironabr,

Ok, thanks for confirming that the performance differences were simply due to differing crypto algorithms.

I'll re-review the proposed change from is/jsch#39 to add an additional flush() call.
I'm thinking that we add some sort of configuration setting to control whether it is performed or not (with the default being to perform the additional flush), so that in case it does cause some sort of regression for other users, they at least will have a way to revert to the old behavior without the additional flush() call.
Do you think that sounds adequate for your use case?

Thanks,
Jeremy

[mironabr]

@norrisjeremy that's sound good. however I don't see cases that this extra flush can cause any regression issue for anybody.
Let me suggest another thing. Your lib is much more modern than the original lib in security perspective. however, for lot of guys that want to upgrade the original lib to your will not be able to connect to old servers because the lack of 'ssh-rsa' support by default. In my case I added a configuration flag for old servers that I added 'ssh-rsa,ssh-dss' to the 'server_host_key' key in the config.
I think it is better to have that config in your code and not in the user code

Regards,
Miron

[mironabr]

@norrisjeremy any update ?
This will help?

#425

[mwiede]

@norrisjeremy that's sound good. however I don't see cases that this extra flush can cause any regression issue for anybody. Let me suggest another thing. Your lib is much more modern than the original lib in security perspective. however, for lot of guys that want to upgrade the original lib to your will not be able to connect to old servers because the lack of 'ssh-rsa' support by default. In my case I added a configuration flag for old servers that I added 'ssh-rsa,ssh-dss' to the 'server_host_key' key in the config. I think it is better to have that config in your code and not in the user code

Regards, Miron

This topic has it's own issue which is #59

[mwiede]

@norrisjeremy any update ? This will help?

#425

@mironabr as you said, that the issue arises with files having 20MB or more, can you provide a failing unit test for this? I think that would proof that it is really a bug.