reconnect callback related issues

[yamt]

while i implemented a reconnect callback for my app, i found a few glitches.
maybe some of them just mean my usage was wrong.
maybe there are a room for improvements in MQTT-C as well.

• MQTT-C reconnect callback can not fail

I don't want to make it reconnect on MQTT_ERROR_SEND_BUFFER_IS_FULL.

also, the underlying transport (in my case, mbedtls) can fail.
(examples/reconnect_subscriber.c just exit in that case. but
it isn't ideal for real applications.)

however, the callback needs to call mqtt_connect anyway because
of the locking protocol.
i workarounded it by having my reconnect callback
call MQTT_PAL_MUTEX_UNLOCK directly when it doesn't want to
or can't reconnect. but I feel it isn't ideal to have
PAL stuff in the user callback.

maybe mqtt_sync should not call the reconnect callback on MQTT_ERROR_SEND_BUFFER_IS_FULL in the first place. i can't imagine the case when it's useful.

maybe we can introduce some api (say, mqtt_reconnect_callback_failed)
to report the failure. (and unlock the mutex)
so that the reconnect callback can do something like the following:

if (no reconnect for now) {
    c->error = something; /* optionally update the error */
    mqtt_reconnect_callback_failed(client);
    return;
}

• mqtt_pal_recvall is called even on an error state

mqtt_sync -> __mqtt_recv -> mqtt_pal_recvall
it means we need to keep client->socketfd valid even on an error state.
(or make mqtt_pal_recvall smart enough to deal with invalid socketfd.
it might not be simple depending on PAL implementations.)
it also means the reconnect callback need to provide valid socketfd
even on failure.

maybe we can skip __mqtt_recv unless MQTT_OK or MQTT_ERROR_SEND_BUFFER_IS_FULL
as __mqtt_send does.

[przemyslawzygmunt]

Try to clear the queue after mqtt_sync ()
https://github.com/SUPLA/supla-core/blob/d2fe9f1b3a74d283c6d47727964e85f80d88fd16/supla-server/src/mqtt/mqtt_client.cpp#L157

After calling mqtt_publish (), check that the error is MQTT_ERROR_SEND_BUFFER_IS_FULL. If so, change to MQTT_OK.
https://github.com/SUPLA/supla-core/blob/d2fe9f1b3a74d283c6d47727964e85f80d88fd16/supla-server/src/mqtt/mqtt_client.cpp#L378

[yamt]

@przemyslawzygmunt thank you.
i have similar workarounds in my app. but i feel it's nicer if these things are handled by the library more naturally.
on the other hand, changing the library behavior might break the existing apps like yours and mine.
how do you think?

[przemyslawzygmunt]

This should be handled by the library but is not. In my opinion, the library needs to be rebuilt not only in this area. Perhaps it is worth starting work on a new version that will not necessarily be compatible with the previous one.

[Pluto-kk]

Hello, I used mqtt in a project and encountered some problems:

1. I cannot use mqtt_error_str to print out "MQTT_OK"
2. When I use client.error == MQTT_OK as the connection condition to judge the connection, I connect to an IP host that does not have the server turned on, and it reconnects twice, and then shows the connection success status
   I think 1, 2 may be caused by a problem, but I don’t know how to solve it temporarily, I hope the author can answer it, thank you

[przemyslawzygmunt]

@Pluto-kk Please show your code.

[LiamBindle]

Hi @yamt thanks for writing. Sorry for no responding earlier.

I don't want to make it reconnect on MQTT_ERROR_SEND_BUFFER_IS_FULL.

This is challenging because a design contraint of MQTT-C is no malloc. Therefore, the send/receive buffers that you initialize the client with is the only block of memory (excluding stack) that MQTT-C uses. You can recover from this event if you're only dealing with QoS 0, but I don't think it's possible for QoS 2 (since behind-the-scenes, the client must enqueue the PUBREL message immediately). The fatal error is specifically the QoS 2 case, so there is room for improvement. I think there's potentially some room fro improvement in freeing up space in the message queue (freeing messages past the first unacked message in the queue). The error handling here isn't trivial, but I'd be happy to merge a safe PR that improves on it.

MQTT-C reconnect callback can not fail

This is true. You can loop inside of your reconnect call back though, until you can re-establish the connection. Again, this is an area has room for improvement, but the error handling isn't trivial. I'd be happy to merge any PRs with improvements.