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

fix EventLoggerPlugin.__getattr__ #92

Merged
merged 1 commit into from Feb 4, 2022

Conversation

edenhaus
Copy link
Contributor

@edenhaus edenhaus commented Jan 30, 2022

The EventLoggerPlugin is implementing wrong the method __getattr__, which lead to an incorrect check in

async def _call_coro(plugin, coro_name, *args, **kwargs):
if not hasattr(plugin.object, coro_name):
# Plugin doesn't implement coro_name
return None
coro = getattr(plugin.object, coro_name)(*args, **kwargs)
return await coro

Before the fix the method hasattr would return True for name like 'topic_filtering' and the method is not returning None. This then will lead to an 'NoneType' object is not callable in line 199.
On version 0.10.0 this creates an infinite wait for the client to subscribe on topics. Bug was find during migration from hbmqtt to amqtt. Probably this PR fixes also #83

The python docs says for object.__getattr__(self, name):

This method should either return the (computed) attribute value or raise an AttributeError exception.

@FlorianLudwig I think we should cherry pick this PR also for the 0.10.x branch as I cannot use the 0.10 without this fix and probably others, if they have disabled topic_check.

@FlorianLudwig
Copy link
Member

@edenhaus Thank you very much for your detailed investigation.

I will need to take a detailed look before merging this. The handling of plugins has interesting edge cases. Especially this is of interest to me:

Bug was find during migration from hbmqtt to amqtt

So you didn't have this bug with hbmqtt?

One of the reasons why I am hesitant to merge this is breaking compatiblity - but it seems we already have.

@edenhaus
Copy link
Contributor Author

edenhaus commented Jan 30, 2022

Hbmqtt has implemented the method __getattr__ wrong and I would fix this correctly. Also to mention this is not a breaking change, it's only a bugfix

@FlorianLudwig The bug was introduces by the following commit a69ef81
As you can see in the diff, the 'NoneType' object is not callable is in hbmqtt catched by except TypeError

@FlorianLudwig
Copy link
Member

@edenhaus Thank you very much! Nice find and investigative work.

The commit introducing the bug actually solved another bug - so this is the right course of action - including for the compatibility branch 0.10.x.

@FlorianLudwig FlorianLudwig merged commit 635e3ad into Yakifo:master Feb 4, 2022
@edenhaus edenhaus deleted the fix-hasattr branch March 6, 2022 12:18
@edmaher
Copy link

edmaher commented Dec 29, 2022

as mentioned in this thread, can we get this cherry-picked to the current major version, the bug is quite a big time-waster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants