server: first pass at Shutdown and Close #34
Conversation
server.go
Outdated
| "time" | ||
|
|
||
| gossh "golang.org/x/crypto/ssh" | ||
| ) | ||
|
|
||
| // ErrServerClosed is returned by the Server's Serve, ListenAndServe, | ||
| // and ListenAndServeTLS methods after a call to Shutdown or Close. | ||
| var ErrServerClosed = errors.New("http: Server closed") |
There was a problem hiding this comment.
Remove references to http
server.go
Outdated
|
|
||
| } | ||
|
|
||
| func (s *Server) shuttingDown() bool { |
There was a problem hiding this comment.
Remove unused function
There was a problem hiding this comment.
If this is removed, we don't need s.inShutdown at all.
server_test.go
Outdated
| timeout := time.After(time.Millisecond * 100) | ||
| select { | ||
| case <-timeout: | ||
| t.Error("timeout") |
server.go
Outdated
| srv.mu.Lock() | ||
| activeConns := len(srv.activeConn) | ||
| srv.mu.Unlock() | ||
| if activeConns == 0 { |
There was a problem hiding this comment.
Refactor activeConn check into function?
server.go
Outdated
| for { | ||
|
|
||
| srv.mu.Lock() | ||
| activeConns := len(srv.activeConn) |
There was a problem hiding this comment.
Since there are no idle connections, maybe we just call them conns?
server_test.go
Outdated
| timeout := time.After(time.Millisecond * 100) | ||
| select { | ||
| case <-timeout: | ||
| t.Error("timeout") |
|
@shazow quick review? |
server.go
Outdated
| // If the provided context expires before the shutdown is complete, | ||
| // then the context's error is returned. | ||
| func (srv *Server) Shutdown(ctx context.Context) error { | ||
| atomic.AddInt32(&srv.inShutdown, 1) |
There was a problem hiding this comment.
Any reason to do this atomically rather than inside of the mu mutex?
(Also looks like inShutdown is not needed anyways?)
| // shutdownPollInterval is how often we poll for quiescence | ||
| // during Server.Shutdown. This is lower during tests, to | ||
| // speed up tests. | ||
| // Ideally we could find a solution that doesn't involve polling, |
There was a problem hiding this comment.
It's not clear to me why polling is necessary? Are we waiting for all of the clients to successfully disconnect before Close() returns? Is this a requirement?
In my code, I typically disable the server from accepting new connections and then in separate goroutine(s) I start issuing disconnect orders/closing connections while the original goroutine just returns.
mattatcha
changed the title
Most of the code is an exact copy of net/http/server.go.
closes: #22