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

Add ability to pass arguments to underlying client's connect method. #56

Merged
merged 2 commits into from
Mar 28, 2021

Conversation

yawor
Copy link
Contributor

@yawor yawor commented Mar 27, 2021

The MQTTv5 protocol no longer uses clean_session setting of the client - it's prohibited by the underlying client to even set this argument when using MQTTv5 protocol. Instead, the MQTTv5 has a clean_start argument which has to be passed to the connect method of the client.
The asyncio-mqtt Client class doesn't provide any way to pass extra arguments to underlying client's connect method. This prevents using (amongst other) the clean_start functionality with MQTTv5 protocol.
This PR proposes a way to set arguments for the underlying paho client connect method, including the clean_start one, by providing a ConnectArgs class containing all the extra args and adds an optional connect_args argument to Client constructor.

An example usage:

async def main():
    async with Client("localhost", protocol=MQTTv5, connect_args=ConnectArgs(clean_start=True)) as client:
        async with client.filtered_messages("foo/#") as messages:
            await client.subscribe("foo/#", options=SubscribeOptions(qos=1))
            async for message in messages:  # type: MQTTMessage
                print(message.topic, message.payload)

@frederikaalund
Copy link
Collaborator

Hi Marcin. Thanks for creating this pull request. Let me have a look. :)

It's a good idea to expose the remaining paho.Client.__init__ arguments. 👍 So far, we've done this via keyword arguments in asyncio_mqtt.Client.__init__. I see that you chose a slightly different approach with the ConnectArgs class as a layer of indirection. I suggest that we remove that layer of indirection and instead expose the remaining arguments directly. E.g.:

class Client:
    def __init__(
        self,
        hostname: str,
        port: int = 1883,
        *,
        # All the existing `paho.Client` arguments
        username: Optional[str] = None,
        password: Optional[str] = None,
        logger: Optional[logging.Logger] = None,
        client_id: Optional[str] = None,
        tls_context: Optional[ssl.SSLContext] = None,
        protocol: Optional[ProtocolVersion] = None,
        will: Optional[Will] = None,
        clean_session: Optional[bool] = None,
        transport: str = "tcp",
        # The new arguments
        keepalive: Optional[int] = None,
        bind_address: Optional[str] = None,
        bind_port: Optional[int] = None,
        clean_start: Optional[bool] = None,
        properties: Optional[Properties] = None,
    ):

What do you think? :)

@yawor
Copy link
Contributor Author

yawor commented Mar 28, 2021

That should be also ok. I wanted to somehow emphasize that these arguments are actually for connect in opposition to the Client instance itself.
But asyncio-mqtt API doesn't do the distinction anyway, as the client starts connection implicitly as a context manager, so maybe there's no need to complicate this. If you take this route then I think it'd be better to also copy default values for these args from the paho.
In the end, what's important is to be able to set these arguments, especially clean_start, as it can affect client logic a lot.

@frederikaalund frederikaalund merged commit 8deaaee into empicano:master Mar 28, 2021
@frederikaalund
Copy link
Collaborator

Thanks, I merged it to master. :)

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.

2 participants