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

HttpUtils abortable requests #6388

Closed
wants to merge 1 commit into from

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Nov 22, 2021

Details

Add functionality to let requests be aborted
Abort any leftover requests during sign out

Fixed Issues

$ #2642

Tests

All platforms

  1. Throttle the network speed from dev tools (e.g. 2kbps download speed)
  2. Accumulate some pending requests by switching chats or switching between tabs
  3. Log out while requests are still running
  4. Observe pending requests are getting canceled

Mobile

Similar to the steps above - we're trying to have pending requests when we log out
On Android, there are throttling options in the Emulator network settings
On iOS there aren't but it's still possible (see the attached video):

  1. Pull down the notifications bar a few times to create pending requests (resulting from app re-focus)
  2. Log out while requests are still running
  3. Observe aborted request logs in the terminal

QA Steps

See if you can do any of the Tests above
If not, try to log in and then log out as fast as possible, there should be no weird behavior afterwards

  • like being unable to login again
  • or any other problems when you log back in

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-11-22.at.12.26.02.mov

Mobile Web

Desktop

iOS

Screen.Recording.2021-11-21.at.22.07.12.mov

Android

@kidroca
Copy link
Contributor Author

kidroca commented Dec 3, 2021

Another interesting case where aborting requests can help is aborting and retrying pending requests after reauthentication

Typical requests when the app is launched with an expired auth token

image

  • 🟨 line represent the end of the 1st authentication call, where we've successfully received the new token
  • 🟧 line highlights a bunch of requests starting before re-authentication is done - they are bound to fail since they use the old token

The 🟧 requests are rejected with expired token which:

  • put all of them back in the queue
  • makes an additional re-authentication which changes the token yet again (sometimes all of this would repeat a 3rd time)

Also note the horizontal grey bars - they represent stalled requests - these are request that Chrome received (our xhr calls), but is too busy to process right now, Chrome allows for up to 6 http 1.1 requests (connections) running at once, other requests would wait to enter the pool - in other words they would only wait to fail due to the expired token

How aborting would help is

  1. When we hit a expired token error and we know that we should reauthenticate
  2. Abort pending requests immediately instead of letting them fail one by one and take precious request queue slots (Chrome's pool)
  3. Schedule the aborted requests to be retried
  4. Prevent new requests from starting while we re-authenticate - we already do this, but for some reason requests will still start shortly before or along with the authenticate call

We should take care to abort only the requests that use an authToken (the "public" requests aren't going to fail)
This PR demonstrates how we can track our own requests and abort all of them, the logic can be easily altered to filter out non authToken bearing requests

@kidroca kidroca force-pushed the kidroca/abortable-requests branch from d315343 to 1be0a68 Compare December 7, 2021 11:51
@kidroca kidroca marked this pull request as ready for review December 7, 2021 11:56
@kidroca kidroca requested a review from a team as a code owner December 7, 2021 11:56
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team December 7, 2021 11:56
@kidroca
Copy link
Contributor Author

kidroca commented Dec 7, 2021

I'm marking this ready so that someone can take a look and decide if it's worth it

Add functionality to let requests be aborted
Abort any leftover requests during sign out
@kidroca kidroca force-pushed the kidroca/abortable-requests branch from 1be0a68 to 3dffba0 Compare December 7, 2021 11:58
@puneetlath
Copy link
Contributor

It seems to work when I test it. But I'm a bit new at this so I think it'd be good to have another set of eyes.

@marcaaron @aldo-expensify @Julesssss looks like you all weighed in on the issue. Any of y'all want to take a look and give an opinion?

@marcaaron
Copy link
Contributor

Unfortunately I'm unable to dedicate any time here.

Looks like a potentially cool idea, but I'm not sure whether we need to do it and the changes are pretty low level which means we will need to be careful in evaluating and merging them.

I'm hesitant in general to change much about the lower level networking stuff at this particular moment in time. The network stuff could really use a refactor or something before adding new features or complexity (to be clear, this doesn't seem crazy complex - just unsure what problem we are trying to solve or if many people are affected by it).

@kidroca
Copy link
Contributor Author

kidroca commented Dec 10, 2021

The original issue that this PR solves expired

Fixed Issues
$ #2642

There's no way to make requests abortable without providing a signal to the low level request handler
Even after a refactor there's no other way but through providing a signal - any other higher level solution would not be halting the actual request

I don't think the change here would affect the app much - existing and future usages of xhr are unaffected - there's no change to the interface

Another reason we need a kill switch like this are tickets dealing with background tasks like

Background tasks on iOS are allowed to run for 30 sec. Exceeding this limit would make the os force close the app
BackgroundFetch would inform us that our time had run out and we need to abort what we're doing

  let isTimeout = event.timeout;  // <-- true when your background-time has expired.
  if (isTimeout) {
    // This task has exceeded its allowed running-time.
    // You must stop what you're doing immediately finish(taskId)
    console.log('[BackgroundFetch] Headless TIMEOUT:', taskId);
    BackgroundFetch.finish(taskId);
    return;
  }

So a functionality like apbortPendingRequests would be handy

BTW with the other change that clears requests from persisted storage (after they're completed), we will just send the remaining requests (including retrying the aborted one) on the next run

@puneetlath
Copy link
Contributor

Hey @kidroca. If you still think this would be good for us to do, I think the ideal thing to do would be take this back to #expensify-open-source and start a discussion there. Then we can decide on whether to move forward. Thanks!

@kidroca
Copy link
Contributor Author

kidroca commented Jan 14, 2022

Thanks for the reply @puneetlath
I'll close this and advertise the feature on slack.

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