Crude implementation of context propagation to Updater #210
Conversation
| @@ -47,8 +47,10 @@ type botMapping struct { | |||
| errorLog *log.Logger | |||
| } | |||
|
|
|||
| var ErrBotAlreadyExists = errors.New("bot already exists in bot mapping") | |||
| var ErrBotUrlPathAlreadyExists = errors.New("url path already exists in bot mapping") | |||
| var ( | |||
| bData.updateWriterControl.Add(1) | ||
| defer bData.updateWriterControl.Done() | ||
|
|
||
| for { | ||
| select { | ||
| case <-bData.stopUpdates: |
There was a problem hiding this comment.
This duplicates the check in shouldStopUpdates
There was a problem hiding this comment.
I don't think we need this; RequestWithContext would handle the exit for us if necessary
| // sentinel, when the context is done, stop all bots. | ||
| <-ctx.Done() | ||
| updater.Stop() | ||
| updater.StopAllBots() |
There was a problem hiding this comment.
Initially just called StopAllBots, but that wasn't enough to stop updater from Idle()'ing.
There was a problem hiding this comment.
Hmm. that might be worth discussing in itself!
There was a problem hiding this comment.
Interesting approach!
Definitely helpful to handle os signals, but I think it misses the usecase of a bot being able to "stop" itself, which I expect is a common usecase (eg, see the multibot example). What do you think?
I've given it my shot in #211; let me know what you think!
| @@ -163,24 +168,31 @@ func (u *Updater) StartPolling(b *gotgbot.Bot, opts *PollingOpts) error { | |||
| } | |||
|
|
|||
| go u.Dispatcher.Start(b, bData.updateChan) | |||
| go u.pollingLoop(bData, reqOpts, v) | |||
| go u.pollingLoop(opts.ParentCtx, bData, reqOpts, v) | |||
There was a problem hiding this comment.
note: this risks a nil pointer if opts is nil :)
| bData.updateWriterControl.Add(1) | ||
| defer bData.updateWriterControl.Done() | ||
|
|
||
| for { | ||
| select { | ||
| case <-bData.stopUpdates: |
There was a problem hiding this comment.
I don't think we need this; RequestWithContext would handle the exit for us if necessary
| // sentinel, when the context is done, stop all bots. | ||
| <-ctx.Done() | ||
| updater.Stop() | ||
| updater.StopAllBots() |
There was a problem hiding this comment.
Hmm. that might be worth discussing in itself!
|
Thanks for your comments! I think #211 nails it, so I'll close this PR. |
What
Propagate parent Context to updater to prevent it idling till the next update when Stop is called.
This is draft and crude, and is intended to be a starting point for discussion.
Impact
observe "Request Failed: context cancelled" in the logs.
but wasn't sure of consequences. What if the request is terminated and that context is dead?