peterhinch / micropython-mqtt

A 'resilient' asynchronous MQTT driver. Recovers from WiFi and broker outages.
MIT License
549 stars 116 forks source link

Connection remains active on exception #74

Closed HCeelen closed 2 years ago

HCeelen commented 2 years ago

mqtt_as.py line 656: In case of an exception in connect(), self._in_connect remains True which overrules self._isconnected However the connection is aborted, isconnected() still returns True

656: Insert self._in_connect = False

peterhinch commented 2 years ago

Please could you clarify. There is no line 656: the last line is line 652.

HCeelen commented 2 years ago

It looks like I’m having another version (0.6.1) In this file it is line 530

If it raises the exception on 531, self._in_connect will not be set to false anymore

I haven’t checked with 0.6.2. connect was called from wifi_connect()

Regards Harold

Van: Peter Hinch @.> Verzonden: Wednesday, 8 June 2022 09:48 Aan: peterhinch/micropython-mqtt @.> CC: Harold Ceelen @.>; Author @.> Onderwerp: Re: [peterhinch/micropython-mqtt] Connection remains active on exception (Issue #74)

Please could you clarify. There is no line 656: the last line is line 652https://github.com/peterhinch/micropython-mqtt/blob/2b01f4a5954da4a6690ee0d8e2aa3da438a2a855/mqtt_as/mqtt_as.py#L652.

— Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython-mqtt/issues/74#issuecomment-1149579790, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AZQUBOGJLPM7V3A5RL6AHDTVOBF27ANCNFSM5YCJFC3A. You are receiving this because you authored the thread.Message ID: @.**@.>>

peterhinch commented 2 years ago

wifi_connect() calls s.connect() where s is the station interface object. It does not call MQTTClient.connect().

MQTTClient.connect() is called from two places.

  1. In user code on initial connection.
  2. In MQTTClient._keep_connected().

In the first case the user presumably attempts to run MQTTClient.connect() again after taking remedial action. In this case the initial state of ._in_connect is a "don't care" as it gets set True unconditionally.

In the second case the exception is trapped and the state of ._in_connect is corrected here.

As far as I can see there is no bug.

peterhinch commented 2 years ago

I assume you're happy with this explanation so closing this.

HCeelen commented 2 years ago

Ok, thanks

Van: Peter Hinch @.> Verzonden: Thursday, 9 June 2022 09:54 Aan: peterhinch/micropython-mqtt @.> CC: Harold Ceelen @.>; Author @.> Onderwerp: Re: [peterhinch/micropython-mqtt] Connection remains active on exception (Issue #74)

I assume you're happy with this explanation so closing this.

— Reply to this email directly, view it on GitHubhttps://github.com/peterhinch/micropython-mqtt/issues/74#issuecomment-1150793641, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AZQUBOCRZWVJQIOKAUY36DDVOGPKBANCNFSM5YCJFC3A. You are receiving this because you authored the thread.Message ID: @.**@.>>

bobveringa commented 2 years ago

Hi,

Apologies for replying to this closed issue; however, I want to add some more context to this issue.

Yes, the in_connect flag is configured correctly if the crash occurs during a reconnect. However, I disagree with that the flag is in a  "don't care" state. Because the function isconnected uses the flag and returns True if it is still set. You are correct that the flag is set to True unconditionally. However, this flag is only set after a Wi-Fi connection is established (unless the client was connected before).

In its current state, an external component calling isconnected() after a crash would return True while this is not the case (even if connected was called again, and there is not yet a Wi-Fi connection). External callers would expect this output of the isconnected function to be correct. However, in its current case, external callers need to have more knowledge about the state of the object. Because, just the result of the function is not enough to determine if the MQTT client is actually connected.

I am interested to hear your thoughts on this @peterhinch

peterhinch commented 2 years ago

I'm not sure of your point. If connectivity with the broker goes down there will be a latency period in which the client has no knowledge of this fact: during that period .isconnected() will return True. When the next ping or publication occurs, it will fail so ._reconnect() is called which sets ._isconnected False. It will remain in that state until reconnection has completed.

It would be possible for .isconnected() to force a ping and query the broker, but to do so it would need to be a coroutine and you'd have to wait for a result. Even then the result would only be valid at that instant in time.

Perhaps I should amend the docs to clarify that .isconnected() can be optimistic. There isn't (so far as I know) a way to get a truly synchronous connected status.

bobveringa commented 2 years ago

The behavior of the .isconnected() seems fine. It is indeed a little optimistic, but I found that if you want to be sure, then you just have to call .broker_up().

If a crash occurs here https://github.com/peterhinch/micropython-mqtt/blob/master/mqtt_as/mqtt_as.py#L541 during first connect. That ._in_connect flag still set to True even if the caller of the connect() functions immediately calls the connect() function again after a crash. The flag is still set to True.

If external components (in our case a LED status checker) makes a call to .isconnected() it will return True, even if there is not yet a valid connection.

This "issue" can be solved by adding self._in_connect = False in the except handler after self.close(). If this is added, calling .isconnected() returns False, as expected.

peterhinch commented 2 years ago

Thanks for that - now fixed.