search

numberoverzero / bottom

asyncio-based rfc2812-compliant IRC Client
http://bottom-docs.readthedocs.io
MIT License
74 stars 23 forks source link

Fix intermittent missing client in Protocol #37

Closed JohanLorenzo closed 7 years ago

JohanLorenzo commented 7 years ago

First of all, thank you for this library! @mozilla-releng uses it for a project called pulse-notify. It allows us to send Firefox's build status via IRC.

We're experiencing an intermittent error (described more thoroughly in bug 1339377). Basically, self.client in protocol is sometime not defined yet. That's because the protocol already handles some requests, even before protocol.client is assigned at this line.

One easy way to trigger this bug, is to connect to irc.mozilla.org. The first message comes in blazing fast, which throws the error immediately.

I didn't manage to write an integration test that points out the bug. I see the bug being fixed manually, though. What slowed me down in writing the bug is the following: The fake server, already passes itself to the protocol, just like this patch does. Hence, I'm not sure of to tackle the test. If you have any suggestion @numberoverzero, I'd be ready to implement.

numberoverzero commented 7 years ago

Thanks for the report and PR! This is a really nice find, and sorry for the inconvenience.

I'm going to merge this now and fix the tests later, because the value of self shouldn't change between the lines Protocol(client=self) call and protocol.client = self -- they're in the same scope. The difficulty of fixing up tests is almost certainly due to the tests themselves, and not because of a common usage pattern.

numberoverzero commented 7 years ago

1.1.0 is released with this fix.

JohanLorenzo commented 7 years ago

Thank you very much for this super fast release! 😄 I'll get this new version deployed on our side.