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: Add longish timeout for polling calls and a shorter one when detecting iOS throttling RELEASE #828

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

shilgapira
Copy link
Member

@shilgapira shilgapira commented Oct 26, 2024

Related Issues

Fixes https://github.com/descope/etc/issues/8031

Description

  • This PR adds a hard timeout of 6 seconds on the calls to /next in the enchanted link polling loop.
  • This ensures that the polling always recovers after a reasonable time if network requests hang for whatever reason – bad routing, dropped packets, throttling (see below).
  • It also adds a mechanism for detecting network request throttling in mobile Safari on iOS devices, possibly only on iOS 18 devices, to fail the poll early and retry as soon as possible.

Throttling

When the flow is running in mobile Safari and the app is in the background, the this.#pollingTimeout = setTimeout(...) callback is not executed until the app returns to the foreground, but the network request that's executed straight after hangs for anywhere between 20-50 seconds.

If we do nothing other than add the timeout then the request will hang for 6 seconds until the timeout fires, and then the request will retry after a further 2 seconds. While this will technically "fix" the bug – the authentication won't be get stuck and the will eventually succeed – the UX will be far from ideal as the user will be waiting for too long.

The throttle code tries to detect whether "too much" time has passed since the timer was scheduled, and if the webpage appears to be hidden, in which case it'll use a much lower timeout of 1 second for the /next API call to fail early. If the request times out as expected, then another polling call is rescheduled to take place 500ms later, instead of 2 seconds as usual. To prevent false positives from causing redundant timeouts in bad network conditions, the code will only use a shorter timeout once, then revert back to the longer one.

By the way, Apple has always had problems in their HTTP layer with throttling network requests in the background. I've been different behaviors in network requests that are fired right after app come back to the foreground in the past, where iOS 11, 12, and 13 each did its own thing. It's possible that this throttling or failure is manifesting in a different way in iOS 18 compared to the past due to some change Apple has made.

Alternatives

  1. We could try to detect the throttling and delay the /next call rather than let it timeout, but this makes for a more brittle code as whatever delay we use might not be enough (I implemented this and around 100ms was in fact not enough), and we'll just end up in the same place.

  2. We could also add a navigator.userAgent check to try to detect that we're running on mobile Safari, but I wasn't 100% sure how to make it precise enough to not have false negatives.

  3. We could also just always use a shorter timeout if document.hidden is true. This would save a lot of this complexity but might cause legitimate calls to fail if the network conditions are not ideal.

  4. Lastly, we could save a lot of the complexity here by defaulting to a short timeout of 1000ms to 1500ms, which is usually enough, and then if the request fails specifically because of a timeout, rerun it once again with a much longer one, say 10 seconds.

Must

  • Tests
  • Documentation (if applicable)

Copy link

vercel bot commented Oct 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
access-key-management-widget ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2024 2:29pm
audit-management-widget ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2024 2:29pm
role-management-widget ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2024 2:29pm
user-management-widget ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2024 2:29pm
user-profile-widget ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 26, 2024 2:29pm

Copy link

nx-cloud bot commented Oct 26, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1c8ba96. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

@dorsha dorsha changed the title Add longish timeout for polling calls and a shorter one when detecting iOS throttling fix: Add longish timeout for polling calls and a shorter one when detecting iOS throttling RELEASE Oct 26, 2024
@shilgapira shilgapira changed the title fix: Add longish timeout for polling calls and a shorter one when detecting iOS throttling RELEASE fix: Add longish timeout for polling calls and a shorter one when detecting iOS throttling Oct 26, 2024
@shilgapira shilgapira changed the title fix: Add longish timeout for polling calls and a shorter one when detecting iOS throttling fix: Add longish timeout for polling calls and a shorter one when detecting iOS throttling RELEASE Oct 26, 2024
@shilgapira shilgapira enabled auto-merge (squash) October 26, 2024 14:37
Copy link
Member

@dorsha dorsha left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Maybe need also @nirgur @asafshen and @itaihanski to review, since they touched this area several times.

@shilgapira shilgapira merged commit 833151e into main Oct 26, 2024
14 checks passed
@shilgapira shilgapira deleted the ios-polling-recovery branch October 26, 2024 14:38
Copy link
Member

@asafshen asafshen left a comment

Choose a reason for hiding this comment

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

looks good, maybe we should have a short chat on this one as well

@shilgapira
Copy link
Member Author

@asafshen We might want to consider whether the 4th alternative I listed might be good enough

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.

3 participants