add read buffer to improve performance

MarketSquare / SSHLibrary

Robot Framework test library for SSH and SFTP

http://marketsquare.github.io/SSHLibrary/

Other

158 stars 139 forks source link

add read buffer to improve performance #431

Closed uruun closed 2 months ago

uruun commented 1 year ago

Add a read buffer to improve performance of read_until and derived functions.

Should fix https://github.com/robotframework/SSHLibrary/issues/425 - I used the tests included in the issue; the execution time of Read Until and pure paramiko is comparable. When used in internal tests I did, it reduced execution time from minutes to seconds.

Closes #425

scott-carrion commented 1 year ago

I just wanted to say: THANK YOU FOR HELPING DEVELOP THIS! This outright fixes performance issues when clients have a very long output, and is even noticeable in the general case.

Maintainers, please merge this PR ASAP!

Many thanks again to uruun, what a legend for helping with this fix

uruun commented 1 year ago

Just found out an issue that this will fail with UnicodeDecodeError - unexpected end of data. Will fix soon.

uruun commented 1 year ago

@rticau should I test it on local environment instead of CI? It seems it cannot find a Ubuntu 18 runner. I would post the results here.

rticau commented 1 year ago

@uruun We are trying to figure out why tests are not running. But meanwhile I guess it wouldn't hurt if you could run the tests yourself. Thanks!

scott-carrion commented 1 year ago

Any update on this pull request? Seems like the CI jobs just need to pass and this can be merged, right?

scott-carrion commented 11 months ago

@rticau , other folks: Can we have this merged? I'm happy to help provide testing info or debug the CI builds. This is a really fantastic improvement.

rticau commented 11 months ago

@scott-carrion Hi. This was not merged as the tests did not pass and I really do not have the time to check. If you can take a look, it would be great.

scott-carrion commented 7 months ago

@rticau , sorry, I haven't had time to either. Is there a way to re-run the CI tests so I can look at the logs and debug?

rticau commented 7 months ago

@scott-carrion I think a small commit would re-trigger the tests (I didn't find an option to trigger manually).

uruun commented 7 months ago

@rticau I pushed a small commit with updated param doc string

uruun commented 7 months ago

Turns out there were some issues, I just pushed fixes, I hope now it passes :)

uruun commented 7 months ago

Some issues I can still see:

The entire build job seems to be failing because robotstatuschecker 1.x uses deprecated functions from robotframework. It works after I installed robotstatuschecker >2.x but it dropped python 2 support. No idea if it will be an issue to use newer robotstatuschecker upstream.
Some tests are failing because of paramiko (probably this https://github.com/paramiko/paramiko/issues/2048)
Some tests are failing and it's expected but robotstatuschecker fails to parse them because it doesn't work at all
No idea about Jython, I didn't test it when I submitted this PR and I don't know why it fails.

Noordsestern commented 2 months ago

Hi,

sorry, I had to merge a big PR for dropping Python2 and Jython support - which also included major restructuring of the entire code. Could you maybe reintegrate your changes in to master? I hope it is just integrating the changes in to client.py .

Thanks

uruun commented 2 months ago

I moved the changes to client.py . Lets try now