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

feat: C SSHNPD --monitor-read-timeout flag #1458

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

JeremyTubongbanua
Copy link
Member

@JeremyTubongbanua JeremyTubongbanua commented Oct 18, 2024

- What I did

  • Implemented --monitor-read-timeout option in C SSHNPD
  • Defaults to 10 seconds

How it works:

Let's say the monitor read timeout is 10 seconds.

Then the monitor connection will do a blocking read on the network buffer for 10 seconds before giving up. Once it gives up, it will send noop:0\r\n to do a check if the monitor connection is alive.

With this new implementation, the monitor connection will only send a noop:0\r\n every monitor-read-timeout seconds, at most.

- Description for the changelog
feat: C SSHNPD --monitor-read-timeout flag

@JeremyTubongbanua
Copy link
Member Author

I added the default constant to sshnpd.h @XavierChanth
Is that the correct place?

@XavierChanth XavierChanth marked this pull request as ready for review October 18, 2024 19:33
@XavierChanth XavierChanth self-assigned this Oct 18, 2024
@XavierChanth XavierChanth requested review from gkc and srieteja and removed request for XavierChanth October 18, 2024 19:33
@XavierChanth
Copy link
Member

Most of the changes were in at_c where I was able to handle the socket code. It turns out mbedtls does have handlers for socket closed, but they shouldn't fully be relied on. We do check for them now, but there are also scenarios where the monitor connection is dead and the socket is still considered open, hence we have configured noop to 45 seconds in this PR.

This noop is only called if the connection has silently failed since the frequency of stats notifications is more often.

@XavierChanth
Copy link
Member

I added the default constant to sshnpd.h @XavierChanth Is that the correct place?

It is the correct place, but I renamed it to main.h to be more appropriate.

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