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

ConnectionManager: always allow event queueing while connecting #1039

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

SimonWoolf
Copy link
Member

Even if was previously suspended or after a failed connection freshness check

The current behaviour is there so that after you go into suspended, and
starts new connection attempts every 30s, it'll still reject publishes
immediately unless/until it becomes connected again. But this results in
some slightly unintuitive behaviour post-device-wake -- an eg presence
enter might fail milliseconds before the sdk reconnects, because it was
never actually having connectivity issues it was just asleep, and it
would have been fine to queue it. Queueing messages for a few seconds
during a connecting cycle (and then failing them if the attempt fails
and the sdk falls back into the suspended state) wouldn't be so bad.

Made the analagous change to the suspended state as well because if you
just remove the connection.queueevents=false line in the freshness
check, but then it fails that and then can't reconnect and goes into
suspended, it would behave differently from if it goes into suspended
via the suspendTimer.

(TBH I've never liked how we mutate the states struct in this way in
various places. IMO would be a lot more elegant to have a completely
frozen states struct, junk the suspendTimer, and decide whether to go
into suspended just by special-casing disconnected in notifystate,
checking how long it's been since we were last connected, and
substituting suspended if it's been over 2 minutes, or something like
that. But that's a change for another time.)

I don't think this change is against the spec, which leaves this ambiguous.

@github-actions github-actions bot temporarily deployed to staging/pull/1039/bundle-report July 28, 2022 18:13 Inactive
@paddybyers
Copy link
Member

I agree that it's messy that (was suspended but is now connecting) is actually a different state from (was disconnected/connected but is now connecting). I think we could tidy this up, but I'm not convinced yet that making them the same state with the same behaviour is going to be sensible in all cases.

@SimonWoolf
Copy link
Member Author

I'm not convinced yet that making them the same state with the same behaviour is going to be sensible in all cases.

To be clear - are you objecting here to the specific change this PR makes (events can always queue during connecting, and if it was suspended they'll be failed when the lib moves back to the suspended state)? Or only to a possible future change to remove all the mutation?

(afaik there's no other behaviour differences except queueEvents + which state is reverts back to if the connection sequence fails)

Even if was previously suspended or after a failed connection freshness check

The current behaviour is there so that after you go into suspended, and
starts new connection attempts every 30s, it'll still reject publishes
immediately unless/until it becomes connected again. But this results in
some slightly unintuitive behaviour post-device-wake -- an eg presence
enter might fail milliseconds before the sdk reconnects, because it was
never actually having connectivity issues it was just asleep, and it
would have been fine to queue it. Queueing messages for a few seconds
during a connecting cycle (and then failing them if the attempt fails
and the sdk falls back into the suspended state) wouldn't be so bad.

Made the analagous change to the suspended state as well because if you
just remove the connection.queueevents=false line in the freshness
check, but then it fails that and then can't reconnect and goes into
suspended, it would behave differently from if it goes into suspended
via the suspendTimer.

(TBH I've never liked how we mutate the states struct in this way in
 various places. IMO would be a lot more elegant to have a completely
 frozen states struct, junk the suspendTimer, and decide whether to go
 into suspended just by special-casing disconnected in notifystate,
 checking how long it's been since we were last connected, and
 substituting suspended if it's been over 2 minutes, or something like
 that. But that's a change for another time.)
@owenpearson owenpearson merged commit 1d1a8f6 into main Aug 8, 2022
@owenpearson owenpearson deleted the always-allow-queueing-while-connecting branch August 8, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants