Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crude implementation of context propagation to Updater #210

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ext/botmapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse my formatter

ErrBotAlreadyExists = errors.New("bot already exists in bot mapping")
ErrBotUrlPathAlreadyExists = errors.New("url path already exists in bot mapping")
)

// addBot Adds a new bot to the botMapping structure.
// Pass an empty urlPath/webhookSecret if using polling instead of webhooks.
Expand Down
18 changes: 15 additions & 3 deletions ext/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ type PollingOpts struct {
// long-polling, Telegram responds to your request as soon as new messages are available.
// When setting this, it is recommended you set your PollingOpts.Timeout value to be slightly bigger (eg, +1).
GetUpdatesOpts *gotgbot.GetUpdatesOpts
// ParentCtx is the parent context for the polling loop.
ParentCtx context.Context
}

// StartPolling starts polling updates from telegram using getUpdates long-polling.
Expand Down Expand Up @@ -155,6 +157,9 @@ func (u *Updater) StartPolling(b *gotgbot.Bot, opts *PollingOpts) error {
v["allowed_updates"] = string(bs)
}
}
if opts.ParentCtx == nil {
opts.ParentCtx = context.Background()
}
}

bData, err := u.botMapping.addBot(b, "", "")
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this risks a nil pointer if opts is nil :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot!


return nil
}

func (u *Updater) pollingLoop(bData *botData, opts *gotgbot.RequestOpts, v map[string]string) {
func (u *Updater) pollingLoop(ctx context.Context, bData *botData, opts *gotgbot.RequestOpts, v map[string]string) {
bData.updateWriterControl.Add(1)
defer bData.updateWriterControl.Done()

for {
select {
case <-bData.stopUpdates:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the check in shouldStopUpdates

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this; RequestWithContext would handle the exit for us if necessary

return
case <-ctx.Done():
return
default:
}
// Check if updater loop has been terminated.
if bData.shouldStopUpdates() {
return
}

// Manually craft the getUpdate calls to improve memory management, reduce json parsing overheads, and
// unnecessary reallocation of url.Values in the polling loop.
r, err := bData.bot.Request("getUpdates", v, nil, opts)
r, err := bData.bot.RequestWithContext(ctx, "getUpdates", v, nil, opts)
if err != nil {
if u.UnhandledErrFunc != nil {
u.UnhandledErrFunc(err)
Expand Down
12 changes: 12 additions & 0 deletions samples/echoBot/main.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package main

import (
"context"
"fmt"
"log"
"net/http"
"os"
"os/signal"
"time"

"github.com/PaulSonOfLars/gotgbot/v2"
Expand All @@ -21,6 +23,9 @@ func main() {
panic("TOKEN environment variable is empty")
}

ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
defer stop()

// Create bot from environment value.
b, err := gotgbot.NewBot(token, &gotgbot.BotOpts{
BotClient: &gotgbot.BaseBotClient{
Expand All @@ -45,13 +50,20 @@ func main() {
MaxRoutines: ext.DefaultMaxRoutines,
})
updater := ext.NewUpdater(dispatcher, nil)
go func() {
// sentinel, when the context is done, stop all bots.
<-ctx.Done()
updater.Stop()
updater.StopAllBots()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially just called StopAllBots, but that wasn't enough to stop updater from Idle()'ing.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. that might be worth discussing in itself!

}()

// Add echo handler to reply to all text messages.
dispatcher.AddHandler(handlers.NewMessage(message.Text, echo))

// Start receiving updates.
err = updater.StartPolling(b, &ext.PollingOpts{
DropPendingUpdates: true,
ParentCtx: ctx,
GetUpdatesOpts: &gotgbot.GetUpdatesOpts{
Timeout: 9,
RequestOpts: &gotgbot.RequestOpts{
Expand Down