Skip to content

Conversation

@amtelekom
Copy link
Contributor

Right now, the handling to global request responses differs between Client and Server.

At the moment, the only situation where we receive those is in response to keepalive requests (everything else is sent with want_reply false), so I've also added a little bit of an explaining comment instead of a leftover "todo".

The old if is not really sufficient to filter for global requests in response to keepalives, since the value it checks, alive_timeouts is reset to 0 with the first response, but due to TCP resends multiple keepalives and their responses might arrive.

This also stops russh client from repeatedly sending an info-level log message about an unknown packet when keepalive responses are received, downgrading unknown messages to debug in general (same as it was in server already), and REQUEST_SUCCESS / REQUEST_FAILURE to trace.

@amtelekom
Copy link
Contributor Author

I guess w.r.t. #148 we could also look into implementing a mechanism for handling global request responses, I'd be open to proposing something if you'd rather handle this

@amtelekom
Copy link
Contributor Author

amtelekom commented Feb 12, 2024

The second commit actually implements global request response handling based on the order and type of global requests sent - although I currently have no setup for in-depth tests of the reverse TcpIp Tunnel stuff, so that is just built by spec without much validation, so maybe someone could validate it a bit further? @jimmyvanhest are you still interested in this stuff?

@amtelekom amtelekom changed the title Global request response logging / Keepalive response handling Global request response handling Feb 12, 2024
@Eugeny
Copy link
Owner

Eugeny commented Feb 15, 2024

The implementation looks good to me - thanks!

@Eugeny Eugeny merged commit 5c60d30 into Eugeny:main Feb 15, 2024
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 this pull request may close these issues.

2 participants