Skip to content

fix: handle Close() before Serve() #248

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kian99
Copy link

@kian99 kian99 commented Apr 16, 2025

This change prevents race conditions in programs that call the server's Close() or Shutdown() methods before Serve(). Consider a program that runs both Serve() and Close() in separate go routines, if a signal comes in early telling the program to shutdown, the Close() method may be called before Serve() has completed its initialisation and begun tracking the listener.

To fix this I've simply copied the latest approach from the Go standard library HTTP server and applied them. Since the SSH server was based on Go's stdlib implementation, bringing the latest stdlib changes was easy. For reference, we can see that in e.g. Go 1.10, there is very little difference between the two. Go 1.11 then removed the logic in trackListener that allowed the server to be reused and Go 1.20 made several changes like the removal of the doneChan in favour of an atomic boolean.

While technically the change/inability to reuse a previously closed SSH server is a breaking one, the same issue was raised for Go's stdlib here and here where it was fixed.

This change prevents race conditions in programs that use the ssh server if they call Close() before Serve(), taking from improvements to the Go stdlib http server.
@kian99
Copy link
Author

kian99 commented Apr 22, 2025

Hi @gustavosbarreto or @belak, would either of you be able to take a look at this?

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.

1 participant