Make client a start/stop service #272
Conversation
brandur
force-pushed
the
brandur-client-start-stop-2
branch
from
fb3c6c6 to
7696b6e
Compare
|
This one a lot more readable without whitespace changes: |
| err := client.Stop(ctx) | ||
| require.Error(t, err) | ||
| require.Equal(t, "client not started", err.Error()) | ||
| }) |
There was a problem hiding this comment.
I originally left this error in, but to guarantee non-raciness, I would've had to an extra mutex to protect it, and it didn't seem worth it since the error's not particularly valuable. (The error wasn't race protected before either, but now we're actually testing it so the problem was revealed.)
| go func() { | ||
| defer close(stopped) |
There was a problem hiding this comment.
It might be more readable and easily debuggable if some of these closures were split into internal client methods. Thinking in particular of the potential for a trace to have anonymous goroutines rather than named methods.
Not a huge concern but it might be worth trying to shuffle some of this around to improve the readability of Start()? Particularly here and in the large startup closure above.
There was a problem hiding this comment.
Took a look at this, and not against it, but I think I'm going to punt for the time being. I kind of like having all the logic Start (and the majority of the stop) logic in one place because you can scan it all at once without having to jump around. Also lets us reduce the number of struct variables and helpers in favor of definitions that can't leak into other contexts, and are defined more visibly, closer to home.
client_test.go
Outdated
| // ctx, cancel := context.WithTimeout(ctx, 10*time.Second) | ||
| // t.Cleanup(cancel) |
There was a problem hiding this comment.
Oops yep. Actually we should talk about this, but the reason I had it commented is that in case your program is actually stuck, the canceled context supersedes go test's -timeout option, so instead of -timeout stopping the test of a full stack trace of every goroutine that was still running, you end up with a canceled test with no debugging information.
So a long way of saying that tighter -timeout parameters are a better option than context timeouts when it comes to use in tests, but in the interest of not getting into that now, I just uncommented these lines.
| } | ||
|
|
||
| // StopInit provides a way to build a more customized Stop implementation. It | ||
| // should be avoided unless there'a an exception reason not to because Stop |
There was a problem hiding this comment.
| // should be avoided unless there'a an exception reason not to because Stop | |
| // should be avoided unless there's an exceptional reason not to because Stop |
There was a problem hiding this comment.
Good catch, thanks.
brandur
force-pushed
the
brandur-fast-completer
branch
3 times, most recently
from
e5d4268 to
71abf47
Compare
brandur
force-pushed
the
brandur-client-start-stop-2
branch
3 times, most recently
from
4091846 to
54fed4c
Compare
Here, continue service refactoring to its ultimate conclusion, and make the client itself a start/stop service, having a couple advantages: * (IMO) considerably simplifies the start and stop code, putting it all in one place, and largely even one function. Fewer helpers to follow around to understand what's going on. * Makes the client behave gracefully on double starts/stops. * Allows the client to easily handle start/stop under duress. Any number of goroutines can be starting or stopping it simultaneously and it's guaranteed free from races. Because the client's stop functions are different from the signature of other start/stop services (taking a context and returning an error), I had to modify the base start/stop service somewhat and allow for an additional `StopInit` helper (like `StartInit`) that lets stop behavior be customized while still providing race-free operations.
brandur
force-pushed
the
brandur-client-start-stop-2
branch
from
54fed4c to
bed599c
Compare
|
ty. |
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress.
* fix StopAndTest after ongoing Stop This regression was introduced in #272, which I noticed because our demo app is not currently cancelling active jobs gracefully before exiting. This commit fixes the behavior by always cancelling the work context upon `StopAndCancel()`, regardless of whether or not another shutdown is in progress. * refactor StopAndCancel tests
Here, continue service refactoring to its ultimate conclusion, and make
the client itself a start/stop service, having a couple advantages:
(IMO) considerably simplifies the start and stop code, putting it all
in one place, and largely even one function. Fewer helpers to follow
around to understand what's going on.
Makes the client behave gracefully on double starts/stops.
Allows the client to easily handle start/stop under duress. Any number
of goroutines can be starting or stopping it simultaneously and it's
guaranteed free from races.
Because the client's stop functions are different from the signature of
other start/stop services (taking a context and returning an error), I
had to modify the base start/stop service somewhat and allow for an
additional
StopInithelper (likeStartInit) that lets stop behaviorbe customized while still providing race-free operations.