StalkR / goircbot

Go IRC Bot
https://godoc.org/github.com/StalkR/goircbot
Apache License 2.0
37 stars 8 forks source link

Reconnect logic drops channels #9

Closed lupine closed 9 years ago

lupine commented 9 years ago

The "disconnected" handler at bot/bot.go:71 rebuilds b.channels from b.Me().Channels() before quitting - presumably, the intent of this is to preserve the effect of joins and parts that have happened since the bot was connected. However, b.Me().Channels() appears to be empty when this handler is run, so the actual effect is to stop the bot from joining any channels at all on reconnection.

Keeping b.channels up to date is probably the right solution in general; my use case is a static list of channels and a long-lived bot, so I can just remove the logic entirely.

StalkR commented 9 years ago

Interesting, thanks for the report.

I also use a static list of channels and long-lived bot. This feature was added by @gelraen in #1. I killed my bot to force a disconnect, it reconnected and joined all the channels.

Update: I upgraded goirc and observe the same behavior now, so it's a change in goirc.

StalkR commented 9 years ago

It's because of https://github.com/fluffle/goirc/commit/3fdd17a2b88b167b0a9ae6d219375bc47cc91f04 which was done because of the issue raised by @sztanpet https://github.com/fluffle/goirc/issues/39

Solutions: 1) remember the channels differently, for instance with a goroutine that saves it periodically 2) revert the change

I think 1) would be a hack and would prefer 2). As @fluffle's originally replied in the issue, the right way to reconnect is with a loop and synchronize with a channel. That's what sp0rkle (https://github.com/fluffle/sp0rkle/blob/master/bot/serverset.go#L35) and goircbot are doing (https://github.com/StalkR/goircbot/blob/master/bot/bot.go#L104). More generally, I think it was good to provide all available information in a "disconnected" event rather than no information at all.

Everyone, thoughts? If you're ok, I've created https://github.com/fluffle/goirc/pull/55 to revert it.

StalkR commented 9 years ago

@lupine: fixed in https://github.com/fluffle/goirc/commit/a3debed5390e7eaa0bfcf93a71019aa63d19b2ea Thanks again for the report!