cmd/trace-agent: remove exit channels in favor of http.Server and context.Context #393
Conversation
gbbr
force-pushed
the
gbbr/no-chan
branch
2 times, most recently
from
0c0754f to
5273743
Compare
cmd/trace-agent/listener.go
Outdated
| // StoppableListener wraps a regular TCPListener with an exit channel so we can exit cleanly from the Serve() loop of our HTTP server | ||
| type StoppableListener struct { | ||
| exit chan struct{} | ||
| // RateLimitedListener wraps a regular TCPListener with an exit channel so we can exit cleanly from the Serve() loop of our HTTP server |
There was a problem hiding this comment.
You can remove reference to the exit channel, since it is no longer true.
cmd/trace-agent/main.go
Outdated
| @@ -24,14 +25,14 @@ import ( | |||
| ) | |||
|
|
|||
| // handleSignal closes a channel to exit cleanly from routines | |||
| func handleSignal(exit chan struct{}) { | |||
| func handleSignal(onCancel func()) { | |||
There was a problem hiding this comment.
I don't like the nameonCancel, it makes me think it's triggered by a cancel event when it's the other way around. onSignal or cancelFunc would seem like more natural names to me.
There was a problem hiding this comment.
Agreed. Changed to onSignal.
cmd/trace-agent/receiver.go
Outdated
| @@ -128,8 +127,7 @@ func (r *HTTPReceiver) Listen(addr, logExtra string) error { | |||
| return fmt.Errorf("cannot listen on %s: %v", addr, err) | |||
| } | |||
|
|
|||
| stoppableListener, err := NewStoppableListener(listener, r.exit, | |||
| r.conf.ConnectionLimit) | |||
| stoppableListener, err := NewRateLimitedListener(listener, r.conf.ConnectionLimit) | |||
There was a problem hiding this comment.
s/stoppableListener/rateLimitedListener/
| func (r *HTTPReceiver) Stop() error { | ||
| expiry := time.Now().Add(20 * time.Second) // give it 20 seconds | ||
| ctx, _ := context.WithDeadline(context.Background(), expiry) | ||
| return r.server.Shutdown(ctx) |
There was a problem hiding this comment.
Seems like a much better way to Stop existing connections than before 👍
cmd/trace-agent/listener.go
Outdated
| // StoppableListener wraps a regular TCPListener with an exit channel so we can exit cleanly from the Serve() loop of our HTTP server | ||
| type StoppableListener struct { | ||
| exit chan struct{} | ||
| // RateLimitedListener wraps a regular TCPListener with an exit channel so we can exit cleanly from the Serve() loop of our HTTP server |
cmd/trace-agent/main.go
Outdated
| @@ -24,14 +25,14 @@ import ( | |||
| ) | |||
|
|
|||
| // handleSignal closes a channel to exit cleanly from routines | |||
| func handleSignal(exit chan struct{}) { | |||
| func handleSignal(onCancel func()) { | |||
There was a problem hiding this comment.
Agreed. Changed to onSignal.
cmd/trace-agent/receiver.go
Outdated
| @@ -128,8 +127,7 @@ func (r *HTTPReceiver) Listen(addr, logExtra string) error { | |||
| return fmt.Errorf("cannot listen on %s: %v", addr, err) | |||
| } | |||
|
|
|||
| stoppableListener, err := NewStoppableListener(listener, r.exit, | |||
| r.conf.ConnectionLimit) | |||
| stoppableListener, err := NewRateLimitedListener(listener, r.conf.ConnectionLimit) | |||
cmd/trace-agent/receiver_test.go
Outdated
| @@ -63,7 +63,7 @@ func TestReceiverRequestBodyLength(t *testing.T) { | |||
| go receiver.Run() | |||
|
|
|||
| defer func() { | |||
| close(receiver.exit) | |||
| receiver.Stop() | |||
| // we need to wait more than on second (time for StoppableListener.Accept | |||
| // to acknowledge the connection has been closed) | |||
| time.Sleep(2 * time.Second) | |||
There was a problem hiding this comment.
Sorry, I didn't spot that on my first read. But is this Sleep still needed?
There was a problem hiding this comment.
I don't know to be honest. The tests pass on both master and this branch without it. Do you reckon I should remove it? We could always add it back if this test turns flaky because of it.
There was a problem hiding this comment.
I think that approach makes sense, we're always better off with less code :)
cmd/trace-agent/agent_test.go
Outdated
| // We need to manually close the receiver as the Run() func | ||
| // should have been broken and interrupted by the watchdog panic | ||
| close(agent.Receiver.exit) | ||
| agent.Receiver.Stop() | ||
| // we need to wait more than on second (time for StoppableListener.Accept |
|
Removed the |
In this change:
exitchannel from StoppableListener and instead just uses(*http.Server).Shutdownfrom the standard library (introduced in Go 1.8)exitchannel from the agent and instead uses the more idiomatic cancellation context.