Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order of disconnection and connection events #64

Merged
merged 2 commits into from
Nov 11, 2022

Conversation

Annopaolo
Copy link
Collaborator

@Annopaolo Annopaolo commented Sep 26, 2022

In rare corner cases (time difference < 10 ms), the on_register hook (reconnection) may be called before the related on_client_offline/gone hook (disconnection). This is due to the distributed nature of VerneMQ (see vernemq/vernemq#1741), and results in an invalid device connection state in Astarte: a device may be publishing while being in a disconnected status.
Introduce a check on the order of disconnection/reconnection events, and possibly reorder them if such a corner case happens.
During reordering (at most 50ms), other kind of messages (if any) are retained in VMQ memory.

Fix astarte-platform/astarte#668.

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #64 (8633c8f) into master (496beb1) will increase coverage by 5.09%.
The diff coverage is 100.00%.

❗ Current head 8633c8f differs from pull request most recent head 9bdb074. Consider uploading reports for the commit 9bdb074 to get more accurate results

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   73.18%   78.28%   +5.09%     
==========================================
  Files           9       11       +2     
  Lines         179      221      +42     
==========================================
+ Hits          131      173      +42     
  Misses         48       48              
Impacted Files Coverage Δ
lib/astarte_vmq_plugin/application.ex 100.00% <ø> (ø)
lib/astarte_vmq_plugin.ex 93.54% <100.00%> (+0.56%) ⬆️
lib/astarte_vmq_plugin/connection/synchronizer.ex 100.00% <100.00%> (ø)
...connection/synchronizer/synchronizer_supervisor.ex 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Annopaolo Annopaolo marked this pull request as ready for review September 26, 2022 13:50
@Annopaolo Annopaolo requested review from rbino and bettio September 26, 2022 13:50
@Annopaolo Annopaolo force-pushed the fix-connection-disorder branch from c737110 to b6709f0 Compare September 27, 2022 12:34
@Annopaolo Annopaolo requested a review from rbino September 28, 2022 14:24
@Annopaolo Annopaolo force-pushed the fix-connection-disorder branch from b6709f0 to a11df0a Compare October 3, 2022 16:45
@Annopaolo Annopaolo requested a review from rbino October 3, 2022 16:46
@Annopaolo Annopaolo force-pushed the fix-connection-disorder branch from a11df0a to 74916df Compare October 4, 2022 09:21
In rare corner cases (time difference < 10 ms), the `on_register` hook
(reconnection) may be called before the related `on_client_offline/gone`
hook (disconnection). This is due to the distributed nature of VerneMQ
(see vernemq/vernemq#1741), and results in an
invalid device connection state in Astarte: a device may be publishing
while being in a disconnected status.
Introduce a check on the order of disconnection/reconnection events, and
possibly reorder them if such a corner case happens.
Fix astarte-platform/astarte#668.

Signed-off-by: Arnaldo Cesco <arnaldo.cesco@secomind.com>
During connection/disconnection synchronization, a device should not be
able to send messages. Make the handling of connection/disconnection
events synchronous, so that VMQ will wait for synchronization before
allowing other messages to be sent.
As the synchronization lasts at most 50ms, this will not affect
standard message flow.

Signed-off-by: Arnaldo Cesco <arnaldo.cesco@secomind.com>
@Annopaolo Annopaolo force-pushed the fix-connection-disorder branch from 8633c8f to 9bdb074 Compare November 11, 2022 16:17
@bettio bettio merged commit 3c1ac84 into astarte-platform:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add temporarily invalid connection state to Known Issues
3 participants