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/1626 handle reachability events #1703

Merged
merged 1 commit into from
May 16, 2023

Conversation

maratal
Copy link
Collaborator

@maratal maratal commented Apr 30, 2023

This is based on #1691
Closes #1626

See commit comments for more details.

@github-actions github-actions bot temporarily deployed to staging/pull/1703/features April 30, 2023 23:02 Inactive
@maratal maratal marked this pull request as draft April 30, 2023 23:03
@github-actions github-actions bot temporarily deployed to staging/pull/1703/jazzydoc April 30, 2023 23:10 Inactive
@maratal maratal force-pushed the fix/1626-handle-reachability-events branch from 55f157d to 40303c0 Compare May 1, 2023 21:32
@github-actions github-actions bot temporarily deployed to staging/pull/1703/features May 1, 2023 21:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1703/jazzydoc May 1, 2023 21:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1703/features May 2, 2023 12:15 Inactive
@maratal maratal force-pushed the fix/1626-handle-reachability-events branch from 83441eb to 29fb344 Compare May 2, 2023 12:19
@github-actions github-actions bot temporarily deployed to staging/pull/1703/features May 2, 2023 12:19 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1703/jazzydoc May 2, 2023 12:25 Inactive
@maratal maratal marked this pull request as ready for review May 2, 2023 13:24
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put a few comments on the code. Also, I found the commits and their messages a bit confusing — 40303c0’s message doesn't make it clear that it's implementing a new feature, it sounds like a bug fix or a refactor or something. And wouldn't it then make sense to put the test in the same commit that implements the feature?

Source/PrivateHeaders/Ably/ARTTestClientOptions.h Outdated Show resolved Hide resolved
Source/ARTRealtime.m Show resolved Hide resolved
Source/ARTRealtime.m Outdated Show resolved Hide resolved
@maratal maratal force-pushed the fix/1626-handle-reachability-events branch from 29fb344 to 08ba4c8 Compare May 6, 2023 18:43
@github-actions github-actions bot temporarily deployed to staging/pull/1703/features May 6, 2023 18:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1703/jazzydoc May 6, 2023 18:48 Inactive
…tart of the connection that have been just started.

There is no certain way to detect whether reachability callback was called because you've just set it up, or because it really has detected reachability of the host. So we rely on a short timeout here - if the callback was called shortly after the setting up reachability, then reconnection shouldn't be done. Whereas if it was called after the certain delay (a threshold) - than it means you are in the middle of the dead-end connection attemt and it should be restarted.

See #1713 for more details.
@maratal maratal merged commit 0bbcfad into main May 16, 2023
@maratal maratal deleted the fix/1626-handle-reachability-events branch May 16, 2023 12:50
This pull request was closed.
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.

Implement RTN20C: Handle os connectivity event while CONNECTING
2 participants