stlehmann / Flask-MQTT

Flask Extension for the MQTT protocol
MIT License
207 stars 71 forks source link

pass disconnect properties to on_disconnect handler #151

Open miketrebilcock opened 1 year ago

miketrebilcock commented 1 year ago

on_disconnect_handler does not pass through the parameters from paho. I need to check the rc parameter to understand the reason for disconnect.

Sohaib90 commented 11 months ago

@miketrebilcock can you provide a little more context to this PR. Why is it required? What did you do to fix the issue? Which tests refer to the change you propose in this PR. Thanks :)

miketrebilcock commented 11 months ago

I was having issues with connections where the server was forceable disconnecting the client. It was passing messages to indicate why the connection was being disconnected, by the library was passing them through to my code. This PR ensures that any data passed in the disconnect is accessible to code that is using the library.

I noticed that some extra commits have managed to be added to the PR, I'll update it to remove them.

Sohaib90 commented 10 months ago

@miketrebilcock can you add some tests to ensure that the changes you have proposed actually work?

howroyd commented 9 months ago

Massive +1 to this PR please.

To add use case as mentioned: The userdata passed into the callbacks in my case has condition variables (threading.Event) to signal an "is_connected" state (rather than using globals.) I can set this variable in on_connect because the userdata structure is passed through, but I cannot clear on_disconnect.

e.g.:

@dataclasses.dataclass
class Userdata:
   connected: mp.Event = dataclasses.field(default_factory=lambda: mp.Event())

@mqtt.on_connect()
def on_connect(client, userdata: Userdata, flags, rc):
    if 0 == rc:
        userdata.connected.set()

@mqtt.on_disconnect()
def on_disconnect(client, userdata: Userdata, rc):  # <- this doesn't work because the args aren't forwarded
    userdata.connected.clear()
miketrebilcock commented 9 months ago

I'll try and get the tests added this week so that it can be merged.