respect provided context in Stop + StopAndCancel #79
Conversation
There was a problem hiding this comment.
I took some liberty in trying to restructure this example based on what I think the intent was. With these methods now respecting the context, you can use a context timeout to make them return early and don't need a separate timer for this purpose. However it was a bit tricky deciding how to also listen for an additional signal to immediately escalate to a hard stop.
It does at least end up being a fair bit shorter after these changes, but please feel free to suggest any improvements based on how you might want this to work.
| @@ -437,8 +437,8 @@ func Test_Client_Stop(t *testing.T) { | |||
|
|
|||
| select { | |||
| case <-jobDoneChan: | |||
| require.FailNow(t, "Expected Stop to return before job was done") | |||
There was a problem hiding this comment.
After this change, we always expect the context to terminate and Stop() to return prior to the job itself actually completing (because it is an intentionally misbehaving job).
bgentry
force-pushed
the
bg-shutdown-respect-context
branch
from
cd63766
to
4f0d0dd
Compare
| @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Changed | |||
There was a problem hiding this comment.
Do you think this should be noted as "breaking" given the potential impact on callers?
There was a problem hiding this comment.
IMO, it might be better to describe how an app might be broken by this change and why we did it.
Might not be the best habit to get into to start including a regular "Breaking" section in the semver doc heh.
| @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Changed | |||
There was a problem hiding this comment.
IMO, it might be better to describe how an app might be broken by this change and why we did it.
Might not be the best habit to get into to start including a regular "Breaking" section in the semver doc heh.
| @@ -105,52 +105,40 @@ func Example_gracefulShutdown() { | |||
| <-sigintOrTerm | |||
| fmt.Printf("Received SIGINT/SIGTERM; initiating soft stop (try to wait for jobs to finish)\n") | |||
|
|
|||
| softStopSucceeded := make(chan struct{}) | |||
| softStopCtx, softStopCtxCancel := context.WithTimeout(ctx, 10*time.Second) | |||
There was a problem hiding this comment.
This example's fully reproduced in the docs, so worth updating it over there too.
An earlier refactor caused these graceful shutdown routines to stop respecting the provided context. No matter what happened with the provided context, they would continue blocking, though they would ultimately return the context's error upon return. With this change, both stop methods will return early if the provided context is cancelled or timed out _prior_ to the clean shutdown completing. This makes it straightforward to implement a shutdown flow which first calls `Stop` with a context that expires after 5 seconds, and after that call `StopAndCancel`. If the shutdown _still_ isn't complete, the user can choose to exit anyway. Partially fixes #78.
bgentry
force-pushed
the
bg-shutdown-respect-context
branch
from
4f0d0dd
to
6ff9984
Compare
An earlier refactor caused these graceful shutdown routines to stop respecting the provided context. No matter what happened with the provided context, they would continue blocking, though they would ultimately return the context's error upon return.
With this change, both stop methods will return early if the provided context is cancelled or timed out prior to the clean shutdown completing. This makes it straightforward to implement a shutdown flow which first calls
Stopwith a context that expires after 5 seconds, and after that callStopAndCancel. If the shutdown still isn't complete, the user can choose to exit anyway.Partially fixes #78. cc @vito