[openhabcloud] Reconnect on connection errors

[ssalonen]

Requires openhab/openhab-osgiify#28

Aiming to resolve issues like openhab/openhab-cloud#134 . Common workaround seems to be to restart the bundle, see https://community.openhab.org/t/myopenhab-service-not-working/121711/5 and https://community.openhab.org/t/connection-to-myopenhab-broken/73808/80.

According to documentation (albeit for 2.x Socket IO version) [1], reconnection is responsibility of the user on connect_error events. Indeed, default implementation (1.0.0 and 1.0.1) only tries to reconnect if it is the first time we are connecting... **EDIT: ** please see #11153 (comment) for more discussion how this behaves in socket io 1.x java client.

[1] Lifecycle diagram in https://socketio.github.io/socket.io-client-java/socket_instance.html

Signed-off-by: Sami Salonen ssalonen@gmail.com

[lolodomo]

Take care, the version is also present in the feature.xml file.
And as a side note, the same library is also used by the ambientweather binding. So it should probably be updated too.

[digitaldan]

Thanks for pointing out the lifecycle documentation and addressing this issue!

[ssalonen]

On closer look, Socket.EVENT_CONNECT_ERROR was named Socket.EVENT_ERROR in 1.x, see https://socketio.github.io/socket.io-client-java/migrating_from_1_x.html. Reading the 1.x codebase carefully [1], one can see that with the default retry-parameters, the Socket/Manager connect_error events are in fact retried automatically by socket.io library. I have tested to the extent possibly by simulating tcp delays and packet loss.

However, Socket.EVENT_ERROR event in 1.0.1 can be also transmitted even though connection is well established, see e.g. https://github.com/socketio/socket.io-client-java/blob/socket.io-client-1.0.1/src/main/java/io/socket/client/Socket.java#L318-L320 -- so it is not a one-to-one replacement of 2.x Socket.EVENT_CONNECT_ERROR.

We can see that in 2.0.1, Socket.EVENT_CONNECT_ERROR is emitted when Manager emits error event and Socket is unconnected:

https://github.com/socketio/socket.io-client-java/blob/socket.io-client-2.0.1/src/main/java/io/socket/client/Socket.java#L91-L98

One way to confirm changed behaviour is to introduce invalid UUID, which is rejected by the server ("not authorized" error). With the new code, client would automatically retry. Old implementation stopped alltogether without retriying.

---

[1] Manager.open() is first called with null OpenCallback. Error events are captured during connection phase by subscribing to engine socket events. Note how this error handler is removed when connection has been successfully subscribed (onopen calls cleanup()).

We can see how connect_error event is emitted in the error handler. Since OpenCallback == null, maybeReconnectOnOpen will be called. The conditional passes and reconnect method is called.

In reconnect method, Manager.open is called again, but now with non-null OpenCallback (!), that is, there is a custom error handler for errors occurring during connect. The custom error handler simply re-connects (with no upper bound by default), and thus side-steps the guards set in maybeReconnectOnOpen.

One can easily test this by blocking traffic to myopenhab.org, and observing how connection is retried, even though connect_error events are emitted.

---

I found it handy to simulate network issues while testing the changes (a bit glitchy as loadbalancer might hand out another ip suddenly):

Introducing 5 sec delay to myopenhab.org

IF=enp0s31f6
myohip=$(nslookup myopenhab.org|grep Address|tail -n1|cut -d ' ' -f2)
sudo tc qdisc del dev  $IF root \
 ; sudo tc filter del dev $IF \
 ; sudo tc qdisc add dev $IF root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
 && sudo tc qdisc add dev $IF parent 1:2 handle 20: netem delay 5000ms \
 && sudo tc filter add dev $IF parent 1:0 protocol ip u32 match ip dst $myohip/32 match ip dport 443 0xffff flowid 1:2

Packet loss

IF=enp0s31f6
myohip=$(nslookup myopenhab.org|grep Address|tail -n1|cut -d ' ' -f2)
sudo tc qdisc del dev  $IF root \
 ; sudo tc filter del dev $IF \
 ; sudo tc qdisc add dev $IF root handle 1: prio priomap 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
 && sudo tc qdisc add dev $IF parent 1:2 handle 20: netem loss 100% \
 && sudo tc filter add dev $IF parent 1:0 protocol ip u32 match ip dst $myohip/32 match ip dport 443 0xffff flowid 1:2

Reseting simulations

IF=enp0s31f6
sudo tc qdisc del dev  $IF root \
; sudo tc filter del dev $IF

[digitaldan]

Hi @ssalonen is this ready to review and try out ?

[ssalonen]

@digitaldan yes, please. No further changes planned at this stage.

[Nils-github]

Maybe the review is picked up faster without WIP in the title?

[ssalonen]

This should not be merged yet since it requires the other PR. Therefore I am keeping the WIP for safety.

[Nils-github]

@ssalonen:
Ok, sorry for my misunderstanding.
Maybe also a "awaiting other PR" label then?

[lolodomo]

The awaiting PR is now merged, isn't it?
@digitaldan @kaikreuzer it is probably time to review

[digitaldan]

thanks @lolodomo for the reminder, i'll take a look.

[digitaldan]

LGTM! I have been running this for the last week without issues, thanks for taking the time to dig into this, i also think the additional debug logging will be helpful for troubleshooting.

[lolodomo]

LGTM

[ssalonen]

i also think the additional debug logging will be helpful for troubleshooting.

Exactly. Not sure if this PR will fix all the issues but hopefully points us to right direction with any open issues.

Btw, I noticed that the socket io client is somewhat old version... I am unsure of these are maintained anymore and if update would be preferable. Unfortunately, this might also imply updating server side...

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/is-home-myopenhab-org-down/130204/7

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/myopenhab-service-not-working/121711/15

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-cloud-connection-stops-working-does-not-try-to-reconnect/133608/17

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-cloud-connection-stops-working-does-not-try-to-reconnect/133608/22