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

Use Rust std TCP set_read_TimeOut instead of a custum one ? #87

Closed
Paulo-21 opened this issue Nov 14, 2023 · 3 comments · Fixed by #89
Closed

Use Rust std TCP set_read_TimeOut instead of a custum one ? #87

Paulo-21 opened this issue Nov 14, 2023 · 3 comments · Fixed by #89

Comments

@Paulo-21
Copy link

Paulo-21 commented Nov 14, 2023

Hey,
I would like to talk about the way you handle the timeout when reading stuff on the tcp connection.

I was wondering if it would be better to use the standard implementation for read with a timeout on TCP rather than a custum one.
https://doc.rust-lang.org/std/net/struct.TcpStream.html#method.set_read_timeout

I m asking my self if your implementation can cause an unnecessary hight CPU usage if the server respond slowly.

fn read_with_timeout<S>(stream: &mut S, tm: Option<Duration>, buf: &mut [u8]) -> SshResult<()>

The tcp connection is set by default to non blocking :

tcp.set_nonblocking(true).unwrap();

So when you read

match stream.read(&mut buf[offset..]) {

The read call terminates immediately if nothing is available at the moment, returning the wouldBlock error, then the loop checks the timeout and loops indefinitely until something is available in the buffer.

It might be more efficient to read only once and let the system manage the timeout ?

What do you think about ?
Because when i connect to my server, i notice an hight cpu usage from my program who use only your lib.

@HsuJv
Copy link
Collaborator

HsuJv commented Nov 15, 2023

I totally agree with that.

But the key is, that the underlying packet processing is performed on any trait that implements Read, ie BufReader, TcpStream, SslStream, WebSocketStream and so on.
I'm not quite sure if optimising for TCP only is sensible.
Perhaps we need to introduce mio?

Anyway, I can do some investigation when I'm free.
Also, any suggestions are welcome.

BRs.

@Paulo-21
Copy link
Author

Paulo-21 commented Nov 15, 2023

Yes, I understand, does the library support or would like another protocol than ssh over tcp?
Mio is for non-blocking IO.
IMO ssh-rs doesn't need to use non-blocking IO calls.
Non-blocking IO is when you want to do things but you're blocked because there's no data available at the time.
As I read it, the library does nothing instead of checking its client's timeout.

I did something experimental, I got a huge improvement in CPU usage from 100% to 0.1%.

You can check with
ssh-rs = { git = "https://github.com/Paulo-21/ssh-rs.git", branch = "main" }
https://github.com/Paulo-21/ssh-rs/commit/f75415f85dea93b6e6c6bbf9d11daab17f61e6c5

Just removing the non blocking by default and set the default timeout to 30 sec with the standard function.
Just the function for changing the timeout of the connection don't even work properly now but we can fix it later

What do you think about ?

@HsuJv HsuJv mentioned this issue Nov 17, 2023
@HsuJv HsuJv closed this as completed in #89 Nov 17, 2023
@HsuJv
Copy link
Collaborator

HsuJv commented Nov 17, 2023

Hi,
I finally fix this high CPU usage via another way
567a88e
which can also make the CPU down to about 0.1-0.5%

And regarding your question:

does the library support or would like another protocol than ssh over TCP?

The answer is Yes. Not only in the WebSocket (In plans), but also in some of my private repos I use it to playback recorded ssh traffic via bufReader

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

Successfully merging a pull request may close this issue.

2 participants