espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
614 stars 258 forks source link

mqtt: allow using external TCP transport handles (IDFGH-10290) #260

Closed huming2207 closed 1 year ago

huming2207 commented 1 year ago

Hi there,

This PR is to add support for external TCP transport handles, e.g. the SOCKS proxy, or my HTTP/HTTPS proxy.

See here for more details: https://github.com/espressif/esp-idf/issues/10684

Regards, Jackson

euripedesrocha commented 1 year ago

Hi @huming2207, I have a similar internal PR that provides the same feature.

We differ in 2 aspects mainly:

I'm attaching the patch with the modifications, so you can take a look. custom_transport.zip

huming2207 commented 1 year ago

Hi @euripedesrocha

We plan to introduce the feature by using directly the transport provided by the user, moving the transport creation to the initialization phase of the state machine instead of client start.

Yea this is a great idea, I really want to implement something like KCP on ESP32 without my current dirty hacks!

We are getting control of the transport and destroying it in case of client destruction. Making it necessary for users to initialize transport again. The decision here is to keep a similar behavior as we have now.

Yep, but I think the user may consider managing the lifecycle of the external TCP transport. One use case for us is like:

  1. We create a proxy handle outside
  2. We try use it to talk on a MQTT API, if it works, it will stay there and does the job we need;
  3. If it fails, it will fall back to a WebSocket API, using the same proxy handle
  4. If it still fails, it will fall back to another HTTPS API.

We haven't implemented all of these features above. But eventually when our project goes well, we wish the proxy connection stays connected as long as possible. Otherwise we may end up millions of devices DDoS'ing our proxy server by opening up the socket and closing it soon after. It also wastes more power and generate more e-waste on our primary battery powered ESP32 devices.

Regards, Jackson

huming2207 commented 1 year ago

@euripedesrocha Another scenario is the NB-IoT network. Unlike Mainland China, the NB-IoT services here in Australia is extremely expensive with a lot of bureaucracy matters we don't want to deal with. We have to try saving the data much as possible. If we have to use an HTTPS proxy via a particular NB network, and because we have no control on the proxy transport, we then have to redo the HTTPS proxy's TLS handshake more often. We will waste a few KB data on each handshake, and we will be charged a lot by the ISP.

euripedesrocha commented 1 year ago

@huming2207 thanks for the information!

Regarding the lifetime, the client

Otherwise we may end up millions of devices DDoS'ing our proxy server by opening up the socket and closing it soon after. It also wastes more power and generate more e-waste on our primary battery powered ESP32 devices.

The transport is destroyed in 2 situations, when the initialization fails and when explicitly called by the user. So we may remove the call to esp_transport_destroy and let the user be responsible for the lifetime and control of the transport.

Even if we keep the call to destroy, you could provide an empty destroy function on your custom transport implementation.

huming2207 commented 1 year ago

The transport is destroyed in 2 situations, when the initialization fails and when explicitly called by the user. So we may remove the call to esp_transport_destroy and let the user be responsible for the lifetime and control of the transport.

Even if we keep the call to destroy, you could provide an empty destroy function on your custom transport implementation.

Hmm yea, I also evaluated a bit further. I think we can control the lifecycle manually by adding some custom dirty hacks in the destroy callback function and config structs. Considering it's more beginner-friendly and less likely to cause memory leaks in generic scenarios, destroying the transport handle by the client is probably better.

euripedesrocha commented 1 year ago

Hi @huming2207, we just merged our version of this feature, can you take a look if it solves your issue?

huming2207 commented 1 year ago

Hi @huming2207, we just merged our version of this feature, can you take a look if it solves your issue?

Ah, thanks!

I think it's working. I will close this pull request for now.