Refactor ExecutionCtx into ExecGroup. #212
Conversation
mikecdavis
requested review from
mikeproeng37,
pawels-optimizely and
thomaszurkan-optimizely
mikeproeng37
left a comment
mikeproeng37
left a comment
There was a problem hiding this comment.
lgtm. I am a fan of not exposing a custom construct to help achieve the same thing.
| // Initialize the default services with the execution context | ||
| if pollingConfigManager, ok := appClient.ConfigManager.(*config.PollingProjectConfigManager); ok { | ||
| pollingConfigManager.Start(appClient.executionCtx) | ||
| eg.Go(pollingConfigManager.Start) |
There was a problem hiding this comment.
I really like how we invert the relationship here
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
Overall, it makes sense to me 👍
|
|
||
| p.startTicker(exeCtx) | ||
| p.startTicker(ctx) | ||
| pLogger.Debug("Batch event processor started") |
There was a problem hiding this comment.
With these changes, does this message actually only get logged when the processor is stopped? Since startTicker was changed to return only when the context signals done.
There was a problem hiding this comment.
Good catch, I'll move the log line.
| if p.EventDispatcher == nil { | ||
| p.EventDispatcher = NewQueueEventDispatcher(exeCtx.GetContext()) | ||
| dispatcher := NewQueueEventDispatcher() | ||
| defer dispatcher.flushEvents() |
There was a problem hiding this comment.
Is this flushEvents necessary? flushEvents is called in startTicker when the context signals done.
There was a problem hiding this comment.
We are aggressively calling flushEvents() that's true. This particular change is to replace the defer in the QueueEventDispatcher "constructor". https://github.com/optimizely/go-sdk/pull/212/files#diff-663642fbf6b03877a468459e9e872cb5L142-L150
| wg.Done() | ||
| }) | ||
|
|
||
| cancel() |
There was a problem hiding this comment.
cool, we were missing that in the ExecutionContext implementation. The only awkward thing will happen when a user will define cancelable context and cancels with the client:
ctx, _ := context.WithCancel(context.Background())
client.Close()
that will return :
the cancel function returned by context.WithCancel should be called, not discarded, to avoid a context leak
pawels-optimizely
left a comment
pawels-optimizely
left a comment
There was a problem hiding this comment.
good implementation of cancelable context.
mikecdavis
changed the title
mikecdavis
force-pushed
the
mikecdavis/OASIS-5660-context-refact
branch
from
d245a2f to
e454957
Compare
mikecdavis
force-pushed
the
mikecdavis/OASIS-5660-context-refact
branch
from
e454957 to
027b174
Compare
Summary
OptimizelyFactoryto accept a plain old context object (POCO)ExecutionCtxintoExecGroup(alaerrgroup)Startmethods to NOT manage their own goroutines, but assume to be executed within one (this is similar tohttp.ListenAndServe)This refactor started when I wanted to manage the
OptimizelyClientlifecycle in my application, but was surprised that I couldn't easily just pass in my own cancellable context. Instead I had to implement my ownExecutableCtx, which also had concepts I didn't feel were immediately relevant to what I was trying to accomplish. Digging further I saw how theExecutableCtxwas being used it followed more of the semantics of an errgroup which means simply managing a "group" of goroutines.An added side effect of this change is that
ExecGroupis isolated to the client pkg and it's not leaked into the other implementations. Everything now just has to deal withcontextand overriding the defaultcontextfeels much more idiomatic.