Page MenuHomePhabricator

[lib] Add fetch timeout to reports service calls
ClosedPublic

Authored by bartek on Sep 3 2023, 11:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 16, 8:03 AM
Unknown Object (File)
Fri, Nov 15, 8:13 PM
Unknown Object (File)
Thu, Nov 7, 4:41 PM
Unknown Object (File)
Thu, Nov 7, 11:51 AM
Unknown Object (File)
Thu, Nov 7, 11:48 AM
Unknown Object (File)
Thu, Nov 7, 11:21 AM
Unknown Object (File)
Oct 30 2024, 2:54 PM
Unknown Object (File)
Oct 30 2024, 2:54 PM
Subscribers

Details

Summary

Raised by @tomek in D9043 - this adds a timeout to the fetch calls to the reports service.

After doing some researc, I've decided to go with AbortController instead of Promise.race. The main advantage of this solution is, that it cancels the fetch request, instead of just ignoring it. This should prevent the request from being executed in the background and potentially consuming bandwidth.

Depends on D9043

Test Plan

Tested on web and native. Used https://httpstat.us/ to generate delayed responses.

For native, it's convenient to add a button to dev-menu.js:

{
  name: 'Test timeout',
  callback: async () => {
    try {
      const response = await fetchWithTimeout('https://httpstat.us/200', {
        timeout: 3000,
        headers: {
          'X-HttpStatus-Sleep': '4000',
        },
      });
      const text = await response.text();
      // for timeout > delay, this should be displayed with "OK"
      Alert.alert('Got response', text);
    } catch (e) {
       // for timeout < delay, this should be displayed with "Request timed out"
      Alert.alert('Got error:', getMessageForException(e));
    }
  },
},

When externalSignal is set, the error message is AbortError: Operation aborted on native and DOMException: The user aborted a request. on web

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Sep 4 2023, 4:32 AM

Have you tested a scenario where externalSignal is used?

This revision is now accepted and ready to land.Sep 4 2023, 5:36 AM
In D9073#267323, @tomek wrote:

Have you tested a scenario where externalSignal is used?

Yes, updated the test plan