PaulSonOfLars / gotgbot

Autogenerated Go wrapper for the telegram API. Inspired by the python-telegram-bot library.
MIT License
509 stars 114 forks source link

Add a NamedHandler, as well as the ability to remove handlers by their name. #47

Closed PaulSonOfLars closed 1 year ago

PaulSonOfLars commented 2 years ago

What

Add a NamedHandler handler, which wraps existing handlers and exposes a stable name to remove them by.

With that, add methods to remove handlers.

And finally... Add some tests to make sure they all work as expected!

Impact

celestix commented 2 years ago

💕 UwU

Nice! I've finally got some stuff to play with once my exams get over 😀

PaulSonOfLars commented 1 year ago

I've put this on hold for now, because I'm not satisfied with the fact these methods aren't thread-safe.

On the other hand, I'm also not a fan of putting everything behind a RWMutex - I worry this would slow down update processing for many, when only a select few actually need to remove handlers.

In my mind, if you need to "remove" handlers, you should use a custom handler and build logic into the CheckUpdate methods to skip handlers which are "disabled", rather than actually remove them and risk concurrency issues. Difficult choices :)

Let me know here if you have any thoughts!

ALiwoto commented 1 year ago

@PaulSonOfLars you are right, this way it does have costs for both people who want the feature, and the ones who don't. So how about adding a boolean field to config when we want to create the bot value then? 🤔

A boolean config var indicating whether the user wants the feature to be enabled or not; how does it sound like? Those who want it, will accept the fact that a RWMutex is being used ( == slower update proccessing, remove handlers feature); but those who don't, simply won't suffer from it.

Let me know if this idea (or something like this) can solve the problem or not! (or, if there is a main problem behind it, other than this)

PaulSonOfLars commented 1 year ago

So how about adding a boolean field to config when we want to create the bot value then? 🤔

A boolean config var indicating whether the user wants the feature to be enabled or not; how does it sound like? Those who want it, will accept the fact that a RWMutex is being used ( == slower update proccessing, remove handlers feature); but those who don't, simply won't suffer from it.

I had a think about this, and the reason I didn't go with it is because that really smells of poorly designed code 😅 And would make it much easier to accidentally forget to use the right locks in the future.

I see another option; make the Dispatcher an interface, and implement it twice. One has removals and mutexes, and the other doesn't. All it needs are Start(...) and Stop() methods. Even allows for others to implement their own if they want to get their hands dirty!

What do you think?

ALiwoto commented 1 year ago

@PaulSonOfLars wow this sounds like a really really good option! Not even it will fix out our current problem, but it will give end-users way way more flexibility by allowing them to even design their own dispatcher! I totally agree with this way :)

vassilit commented 1 year ago

RWMutex - I worry this would slow down [...] make the Dispatcher an interface

Converting to an interface will induce some overhead (allocation).

Also, there are alternatives to RWMutexes: