go-telegram-bot-api / telegram-bot-api

Golang bindings for the Telegram Bot API
https://go-telegram-bot-api.dev
MIT License
5.72k stars 882 forks source link

Multi-thread safe? #273

Closed utdrmac closed 4 years ago

utdrmac commented 4 years ago

Just wondering if I need to protect the bot object with a mutex when calling bot.Send() from multiple go routines. I don't think I do, but just want to be sure.

fabulias commented 4 years ago

I think that is necessary. Please read the code inside this method

func (bot *BotAPI) uploadAndSend(method string, config Fileable) (Message, error)

utdrmac commented 4 years ago

Ok. I read that code. Don't see why I need mutex protection. Can you explain? What if I'm not sending files/images? I'm just doing regular chats.

fabulias commented 4 years ago

you never must work assuming that if a way is thread-safe, all ways will be, I think that is necessary add a mutex for example...

resp, err := bot.UploadFile(method, params, config.name(), file)
    if err != nil {
        return Message{}, err
    }

In that code... what assures me here that the method UploadFile is not used for two goroutines at the same time and was upload correctly the file? This is a case, there may be many more cases.

utdrmac commented 4 years ago

mybot := NewBotAPI(key) go func() { mybot.UploadFile(.. A ..) } go func() { mybot.UploadFile(.. B ..) }

I don't see how this could make UploadFile(A) return inside the go func of B. When UploadFile(A) is ran, all the parameters go to that function and become local scope. Same for UploadFile(B). There's no way scope of B will return or impact scope of A.

Do you see this some other way?

IzeBerg commented 4 years ago

I strictly recommend learning the basics of Golang. In this case, understanding functions and closures can help you: https://golang.org/ref/spec#Function_literals https://golang.org/ref/spec#Function_declarations https://golangcode.com/anonymous-functions/

You can't write effective code without questions like this without knowing about basics.

utdrmac commented 4 years ago

I've got the basics of Golang down pretty good since I've been using it since 1.4. (my above example was just go-pseudo-code and not real go code) Still not seeing an issue of calling procedural bot functions from multiple go routines, especially when the values are being returned back to the caller that called originally. If you could provide some example code where calling various bot APIs breaks down in a multi-gofunc down situation, where a mutex would be required, that would be helpful.

IzeBerg commented 4 years ago

Git gut. It doesn't matter your experience of working with Golang if you don't know how to resolve your tasks with it.

    mybot, _ := tgbotapi.NewBotAPI(key)
    var wait sync.WaitGroup
    var msg1, msg2 tgbotapi.Message
    var err1, err2 error
    wait.Add(1)
    go func() {
        defer wait.Done()
        msg1, err1 = mybot.Send(msg)
    }()
    wait.Add(1)
    go func() {
        defer wait.Done()
        msg2, err2 = mybot.Send(msg)
    }()
// enjoy msg1, msg2, err1, err2 as results

You may not need "thread-safe" unless you need rate-limit.

utdrmac commented 4 years ago

Ok. I see the issue and also the confusion. Here's my modified code from what you gave. Thank you for that. Yes, your example has an issue where multiple messages sent to the same user may be received out of order. I experienced this when I ran the code below. Message 2 was received before Message 1. In my particular use-case, however, this can never happen. I'll never send more than 1 message at a time to a single user. I was concerned about protecting bot.Send() when sending many messages "at the same time" but to multiple users. It does not appear that this is an issue after all.

$ cat multibot.go
// +build ignore
package main

import (
    "log"
    "sync"
    tbot "github.com/go-telegram-bot-api/telegram-bot-api"
)

func main() {

    mybot, _ := tbot.NewBotAPI("XWYXWYXWYXYW")

    var wait sync.WaitGroup
    var msg1, msg2 tbot.Message
    var err1, err2 error

    wait.Add(1)
    go func() {
        defer wait.Done()
        msg := tbot.NewMessage(438991111, "Message 1")
        msg1, err1 = mybot.Send(msg)
    }()

    wait.Add(1)
    go func() {
        defer wait.Done()
        msg := tbot.NewMessage(438991111, "Message 2")
        msg2, err2 = mybot.Send(msg)
    }()

    wait.Wait()

    log.Println("MSG1:", msg1)
    log.Println("ERR1:", err1)

    log.Println("MSG2:", msg2)
    log.Println("ERR2:", err2)
}
IzeBerg commented 4 years ago

What's the point? If you need strict ordering -- so order it by yourself (collect messages before send and send in your order, or something other). "Thread-safe" does not mean the strict order of actions, it means safe to use in different threads at the same time. I think you don't understand how threads/goroutines works or even you don't know what you need to do.

utdrmac commented 4 years ago

I don't need strict ordering. I am not sending multiple messages to same user. That's what I've been saying. I'm sending a unique message to multiple users from various go-routines. Yes, I know what thread-safe means, could you please stop with the ad-hominem attacks and focus instead on the original question? I've got 4 different go-funcs that are all doing various processing of events from external sources. When certain conditions happen within those, send a message to user with bot.Send(). I don't care about ordering. There will never be a case where user1 will receive a message from multiple go-funcs so there's no ordering issue. I do know what I need to do because this code is already working just fine in production. However, just because there hasn't been an issue yet doesn't mean there won't be, so if I needed to, I'd start protecting with mutex now. From all of this, it does not appear that I need to protect bot.Send() from multiple callers within different go-funcs. Agree?

IzeBerg commented 4 years ago

It depends on the count of messages. Telegram hasn't rate limit on requests. I'm using a queuing when mailing and handle timeout errors. Another way - mutex, but for me, it's too uncontrollable. This whole discussion not an issue of the package, it is about the implementation of your specific functionality. Also sorry, but "attacks" are based on your words, question determines the answer.

utdrmac commented 4 years ago

I'm just following the model I've used with other API kits. Most of them tell you up front that if you are making calls to the same API function from multiple go-funcs, you'll need (or not need) to protect the call with a mutex, otherwise the internal state of the package can be messed up. I don't see anything on the main README that gives indication I need to do this, so that's why I'm asking.

iofik commented 1 year ago

Was it resolved? I could not find the answer to this in the docs. I think the most significant object in the BotAPI is HTTPClient, which is said to be thread-safe, but it would be better to have a statement in the official docs. In particular, does HTTPClient guarantee that responses are received in correct order and returned to correct places if multiple requests are run simultaneously? And I must say, it would be really annoying if I have to use a mutex for every request: waiting for full round-trip before moving to the next request looks very limiting.