Add property & setter: Client.create_socket

[JamesParrott]

Implements #873

I'm raising this as a draft PR now to gather feedback. I'll add tests in due course.

[PierreF]

The issue I see with adding API to simply replacing _create_socket is that this function does lots of task:

• It connect to unix OR TCP server
• It select the host & port to connect to (and the timeout and the source address). If multi-addresses broker support is added it might cause conflict here
• It handle proxy (HTTP & SOCKS)
• It handle TLS/SSL
• It handle websocket

So we need to carefully pick the API we use. I believe we shouldn't replace the full _create_socket as it would either means:

• we can't assume all setup listed above are done, which might cause other issue (lots of attribute, self._host, self._transport, ... can't be trusted to be right)
• user provided version must implement all of this

But maybe the right level would be to be a "proxy" level:

• Either user set a "classic" proxy (or no proxy) - today behavior
• Or use a function proxy, which create the TCP connection. So it's the function that replace socket.create_connection with all its arguments

This way most feature of _create_socket are kept, and I think it make sense to be at "proxy" level for this feature.

I think think to a set_create_connection() which take a function create_connection(addr, timeout=self._connect_timeout, source_address=source). It's documented to only be used for for TCP connection (not unix socket) and that TLS/SSL and websocket is added after (which is not configured isn't done).

Does this sound good to you ?

[JamesParrott]

Thanks for your thoughts, Pierre.

I fully understand where you're coming from - _create_socket can call to three sub methods (and _WebsocketWrapper), giving 6 possible combinations (unix, tcp & websockets, all three both with and without ssl). Plus possibly handling a proxy.

But, as far as I can tell (please correct me if I'm wrong), that method is essentially a factory that returns one of 10 possible combinations. None of those methods alter the client's state. Given a few extra args for the client's attrs, they could all be refactored into functions that return some socket (I'm not suggesting this). _proxy_is_valid is already a static method.

My idea is, that instead of in future adding even more config and custom logic, to implement an 11th, a 12th, or even a 20th socket, lets just let those users that want those sockets, create them and supply them themselves. All they need to do it implement the SocketLike.

I completely agree that any "user provided version must implement all of this". The intended user base for this feature would be required to handle all that for themselves, whilst creating their custom socket. And LibraryRaven for one, is telling us that they're not only willing to do so, they have already done this. And perhaps other third party libraries, as well as Paramiko even provide socket instances out of the box already, that with a few tweaks, Paho.mqtt.Python should happily work with.

I'll bow to your experience though, if you think officially supporting this would create even more issues and headaches from such power users in future.

[PierreF]

The issue I see with replacing _create_socket with a user-provided function, is that either:

• we have multiple attribute that are likely wrong (self._host, self._ssl, self._transport, self._tls_insecure).
• or the user-provided function will need some hack (access to private field or pass them twice hoping nothing de-synchronize them)

My initial idea is that the provided user-function should be able to replace a existing function that don't access private fields, hence my suggestion to connect to "proxy-level".

On the other hand, I agree that being at "SocketLike" interface is better then being a "TCP proxy opened" as it avoid to assume TCP. It opens to more possibility.

But I think a good interface should allow to re-implement the internal paho socket creation with a user-provided implementation. For example to reimplement _ssl_wrap_socket but using another SSL implementation. Which means that _ssl_wrap_socket should be implementable without accessing private field/method. Possibly passing an argument to the create_socket function which contains all details ?

My idea could be a signature like create_socket(info: ConnectionInfo) -> SocketLike with ConnectionInfo being a structure with the host, port, ssl + ssl details (insecure, ssl_context), proxy hints ?. This way, the user-implementation could works exactly as paho's implementation, likely because the updated paho implementation could access data from the info parameter rather than from a self._variable.

Going this way, I would probably include two other point:

• the _ssl_wrap_socket might be useful to user-implementation. In other to prove our interface, I would move it to plain function def ssl_wrap_socket(bottom_sock: SocketLike, info: ConnectionInfo) -> SocketLike. This way paho would use it but user-implementation could also.
• the self._transport should be "custom" (or the function itself ?) when a custom connection factory is provided. This would solve the inconsistency of self._transport that could be "websockets" when you actually have an tcp+ssh. I'm fine with changing self._transport to "custom" when setting self.create_socket or that changing the create_socket goes through the self.transport setter (the later is a bit odd in that the setter would take a str | SocketFactory).

Obviously with this interface, the user create_socket have access to all details (host, port, ssl settings) and could use them. If they don't, it's up to the user. But they have everything in their hand to handle / validate them (e.g. apply the settings, silently ignore them even if it might cause unexpected result, or maybe raise error for unsupported settings).