Implement graceful shutdown and propagate request context #468
Conversation
johejo
requested review from
JoelSpeed,
steakunderscore and
loshz
as code owners
http.go
Outdated
| sigint := make(chan os.Signal, 1) | ||
| signal.Notify(sigint, os.Interrupt, syscall.SIGTERM) | ||
| s := <-sigint | ||
| logger.Printf("caught signal: %s", s) |
There was a problem hiding this comment.
Is it worth printing what kind of signal this is?
Also, might be worth adding to this that we are now going to shutdown?
There was a problem hiding this comment.
I was looking at this log in testing.
This is certainly not necessary, as you say.
I'll remove it.
| logger.Printf("caught signal: %s", s) | ||
|
|
||
| // We received an interrupt signal, shut down. | ||
| if err := srv.Shutdown(context.Background()); err != nil { |
There was a problem hiding this comment.
Will this panic if the server never started correctly?
There was a problem hiding this comment.
It looks like ErrServerClosed is returned even if the server is not up.
https://github.com/golang/go/blob/master/src/net/http/server.go#L2905
| "github.com/oauth2-proxy/oauth2-proxy/pkg/logger" | ||
| fsnotify "gopkg.in/fsnotify/fsnotify.v1" |
There was a problem hiding this comment.
I'm not sure about this change, I've definitely seen dependency problems in the past, can you double check this should be being done?
There was a problem hiding this comment.
Is this what you're referring to as problem?
fsnotify/fsnotify#328
fsnotify/fsnotify#309
I'll upgrade to v1.4.9 that supports go mod officially.
| @@ -290,7 +293,7 @@ func newTicket() (*TicketData, error) { | |||
| return nil, fmt.Errorf("failed to create new ticket ID %s", err) | |||
| } | |||
| // ticketID is hex encoded | |||
| ticketID := fmt.Sprintf("%x", rawID) | |||
| ticketID := hex.EncodeToString(rawID) | |||
There was a problem hiding this comment.
Does this change the output at all? Just concerned about existing tickets
There was a problem hiding this comment.
I think this change will return exactly the same result.
If you do not like it, I will back to the original implementation.
https://play.golang.org/p/ijrr87i_rox
http_test.go
Outdated
| srv.ServeHTTP() | ||
| }() | ||
| time.Sleep(500 * time.Millisecond) | ||
| if err := syscall.Kill(os.Getpid(), signal); err != nil { |
There was a problem hiding this comment.
Is this sending a kill signal to the test suite? Does this not cause the test suite to fail? Is there no other way to simulate the signal?
http.go
Outdated
| sigint := make(chan os.Signal, 1) | ||
| signal.Notify(sigint, os.Interrupt, syscall.SIGTERM) | ||
| s := <-sigint | ||
| logger.Printf("caught signal: %s", s) |
There was a problem hiding this comment.
I wonder if it's better to set up the stop channel in main.go and pass this through, that way it might be easier to test?
johejo
force-pushed
the
graceful_shutdown
branch
from
0af3829
to
0bc2441
Compare
http_test.go
Outdated
| }() | ||
|
|
||
| stop <- struct{}{} // emulate catching signals | ||
| wg.Wait() |
There was a problem hiding this comment.
In case this functionality is broken, this test case would hang right? Do you think we can add a timeout somehow?
There was a problem hiding this comment.
To find broken case, I added assert to check channel length and add an idiomatic for sync.WaitGroup with timeout.
Propagate the request context to the Redis client. It is possible to propagate a context cancel to Redis client if the connection is closed by the HTTP client. The redis.Cmdable cannot use WithContext, so added the Client interface to handle redis.Client and redis.ClusterClient transparently. Added handling of Unix signals to http server. Upgrade go-redis/redis to v7.
- Upgrade golang/x/* and google-api-go - Migrate fsnotify import from gopkg.in to github.com - Replace bmizerany/assert with stretchr/testify/assert
johejo
force-pushed
the
graceful_shutdown
branch
from
0bc2441
to
7c2ea54
Compare
Description
Propagate the request context to the Redis client.
It is possible to propagate a context cancel to Redis client if the connection is closed by the HTTP client.
The redis.Cmdable cannot use WithContext, so added the Client interface to handle redis.Client and redis.ClusterClient transparently.
Added handling of Unix signals to http server.
Upgrade go-redis/redis to v7.
Notes
Motivation and Context
In container environment requires more graceful behavior.
Proper handling is also required when disconnected from HTTP clients and forward proxies.
If the context is not propagated properly to read and write to Redis session storage, there is a possibility of unexpected trouble.
How Has This Been Tested?
Added graceful shutdown test.
Checklist: