allow to use any port as TLS port

[vladak]

Instead of using the MQTT_TLS_PORT value as indication whether TLS should be used, this change introduces private field that is set based on the MQTT() is_ssl argument.

The downside of this approach is that if someone specifies port=1883 explicitly without setting is_ssl to False, the port will be treated as TLS port since the is_ssl is True by default and OSError("Failed SSL handshake") will be thrown, unless the broker is configured to accept TLS on that port. I have not made my mind yet whether to special case it and force to insecure so leaving it open for the reviewers.

[FoamyGuy]

Thank you for working on this @vladak!

I do think we want to have port 1883 be a special case that will use is_ssl=False even when the user didn't pass in is_ssl. Maybe we could have that default to None instead of True, and then if user didn't set it have it get set based on the port number (which does still default to ssl port).

The learn guide for mini mqtt that uses the example code from here directs people to test with adafruit.io and it's using 1883. So the examples don't work by default with this change, we could theoretically add is_ssl=False to them as a fix, but I think it's more likely to be expected by users that 1883 will be non-ssl by default.

[FoamyGuy]

As written this argument doesn't actually work if user attempts to set it to False In that case if is_ssl: evaluates to False and therefore self._is_ssl never gets created and there is a exception raised when the code first tries to access it.

I tested one possible fix by changing this line and moving it to above the if statement like this:

        self._is_ssl = is_ssl
        self.port = MQTT_TCP_PORT
        if is_ssl:
            self.port = MQTT_TLS_PORT
        if port:
            self.port = port

This way self._is_ssl will always get created and set to the value that the user passed in.

[vladak]

Thanks ! I used that as a basis, hopefully it looks better now.

[vladak]

Also, I added buch of tests (wink #64) that will hopefully make it more clear.

[vladak]

Note that with the changed code, there is a change of behavior - the is_ssl has to be specified explicitly even for the default TLS port.

[FoamyGuy]

Looks good to me. Thanks for the improvement @vladak! Also for adding a test to validate the port behavior.

I tested the latest version successfully with the native networking aio example in the repo.