Add support for cleanly shutting down a server #23
Conversation
belak
force-pushed
the
belak/drain-connections
branch
from
be0f343
to
6cd286b
Compare
|
Maybe something like srv.Shutdown(ctx) should be used (see https://tylerchr.blog/golang-18-whats-coming/) |
belak
force-pushed
the
belak/drain-connections
branch
from
b976c81
to
a6e8c73
Compare
server.go
Outdated
| } | ||
|
|
||
| // getKillChan is used to get or create the kill channel | ||
| func (srv *Server) getKillChan() chan struct{} { |
There was a problem hiding this comment.
Why would we make this seem like magic instead of having an explicit initialization of the killChan? It seems like this could result in somebody not realizing there are side-effects here.
There was a problem hiding this comment.
I generally agree, but I'm also a lot more tolerant when it comes to internal APIs for small projects.
There was a problem hiding this comment.
This is also similar to what net/http does. The locks and such are used more there because it's more complicated, but the general idea is the same. A large portion of this takes inspiration from there.
There was a problem hiding this comment.
Oh, but also thanks for reviewing @mark-adams !
There was a problem hiding this comment.
Now that I've read the full source, I think it does makes sense to not have this (or getDoneChan) and instead initialize in the one place these are called: when entering stateStarted in Serve().
server.go
Outdated
| } | ||
|
|
||
| // getKillChan is used to get or create the wait group | ||
| func (srv *Server) getWaitGroup() *sync.WaitGroup { |
There was a problem hiding this comment.
Like Mutex, a WaitGroup can be used as an empty value. Even the example does it this way: https://golang.org/pkg/sync/#example_WaitGroup
There was a problem hiding this comment.
I've removed this helper function, but I still think it should be a pointer, otherwise there's a race condition between shutting down a server and starting a new one with the same Server object.
| srv.listener = nil | ||
|
|
||
| // Grab the waitgroup then unlock | ||
| wg := srv.wg |
There was a problem hiding this comment.
This shouldn't be necessary once changed to a value.
server.go
Outdated
| } | ||
| } | ||
|
|
||
| // getKillChan is used to get or create the done channel |
There was a problem hiding this comment.
Comment name discrepancy. (Though irrelevant if removed)
There was a problem hiding this comment.
All these helpers have been removed.
server.go
Outdated
| } | ||
|
|
||
| // getKillChan is used to get or create the kill channel | ||
| func (srv *Server) getKillChan() chan struct{} { |
There was a problem hiding this comment.
Now that I've read the full source, I think it does makes sense to not have this (or getDoneChan) and instead initialize in the one place these are called: when entering stateStarted in Serve().
| srv.listener = nil | ||
|
|
||
| // Grab the waitgroup then unlock | ||
| wg := srv.wg |
belak
force-pushed
the
belak/drain-connections
branch
from
24b7d6c
to
50041da
Compare
belak
changed the title
|
Once we agree on the general method, I'll add some tests. |
|
Note to self: there's a small issue currently around the state getting set to stateClosing while the server is still draining. EDIT: fixed |
belak
force-pushed
the
belak/drain-connections
branch
from
f3edba7
to
3b925b3
Compare
This is related to gliderlabs#22 and gliderlabs#20
belak
force-pushed
the
belak/drain-connections
branch
from
3b925b3
to
adfe26c
Compare
belak
force-pushed
the
belak/drain-connections
branch
from
66dd10b
to
0578bf7
Compare
|
There's a race condition somewhere (no surprise there) and the tests are failing sometimes.
Ideas would be appreciated. |
|
Shall we close in favor of #34? |
|
That's probably the best move. I'd like to get this in as it doesn't rely on polling to shut down, but the other method should work. |
|
Okay, well thanks for the contribution in any case and instigating a solution for this. |
This is related to #22 and #20 and is needed to write tests.
I understand this is probably more complicated than we were hoping, so feel free to offer suggestions for improvements.
Please do not merge this until I get a chance to double check stuff and clean it up.