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

Reachability class prone to indicate a false positive of client's connectivity #13206

Open
3 tasks done
luisenaguero opened this issue Apr 1, 2024 · 4 comments
Open
3 tasks done
Assignees
Labels
DataStore Related to DataStore category feature-request Request a new feature

Comments

@luisenaguero
Copy link

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

DataStore

Amplify Version

v5

Amplify Categories

api

Backend

Amplify CLI

Environment information


  System:
    OS: macOS 14.3.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 36.72 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.0 - ~/.nvm/versions/node/v18.17.0/bin/node
    Yarn: 1.18.0 - ~/.nvm/versions/node/v18.17.0/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.0/bin/npm
  Browsers:
    Chrome: 123.0.6312.59
    Edge: 123.0.2420.53
    Safari: 17.3.1
  npmPackages:
    @aws-amplify/api: 5.0.5 => 5.0.5 
    @aws-amplify/cli: 10.5.2 => 10.5.2 
    @aws-amplify/datastore: 4.0.5 => 4.0.5 
    @aws-amplify/storage: 5.0.5 => 5.0.5 
    @aws-amplify/ui: 2.0.5 => 2.0.5 
    @babel/plugin-proposal-class-properties: ^7.10.1 => 7.18.6 (7.8.3)
    @babel/plugin-transform-flow-strip-types: ^7.21.0 => 7.21.0 (7.9.0, 7.23.3)
    @commitlint/cli: ^17.6.1 => 17.6.3 
    @commitlint/config-conventional: ^17.6.1 => 17.6.3 
    @pmmmwh/react-refresh-webpack-plugin: ^0.5.7 => 0.5.10 
    @react-native-community/eslint-config: ^3.1.0 => 3.2.0 
    @react-native-community/eslint-plugin: ^1.1.0 => 1.3.0 
    @react-native-community/toolbar-android: ^0.1.0-rc.2 => 0.1.0-rc.2 
    @testing-library/react: ^14.0.0 => 14.0.0 
    @testing-library/react-native: ^5.0.3 => 5.0.3 
    @testing-library/user-event: ^14.4.3 => 14.4.3 
    @types/alucio-core: 0.0.1 => 0.0.1 
    @types/jest: ^29.5.0 => 29.5.1 
    @types/new-relic-browser: ^0.1118.1 => 0.1118.2 
    @types/react: ^18.0.24 => 18.2.6 
    @types/react-dom: ^18.0.11 => 18.2.4 
    @types/react-native: ^0.71.4 => 0.71.7 (0.70.13)
    @types/react-native-vector-icons: ^6.4.13 => 6.4.13 
    @types/react-router: ^5.1.19 => 5.1.20 
    @types/react-router-dom: ^5.1.5 => 5.3.3 
    @types/react-router-native: ^5.1.0 => 5.1.3 
    @types/segment-analytics: ^0.0.33 => 0.0.33 
    @types/uuid: ^8.3.4 => 8.3.4 (8.3.0, 3.4.10)
    @typescript-eslint/eslint-plugin: ^5.40.0 => 5.59.7 
    @typescript-eslint/parser: ^5.40.0 => 5.59.7 
    HelloWorld:  0.0.1 
    babel-jest: ^29.5.0 => 29.5.0 
    chalk: ^4.1.0 => 4.1.2 (2.4.2, 4.1.0, 3.0.0, 1.1.3)
    cross-env: ^7.0.2 => 7.0.3 
    cz-conventional-changelog: 3.3.0 => 3.3.0 
    eslint: ^7.22.0 => 7.32.0 
    eslint-config-standard: ^14.1.1 => 14.1.1 
    eslint-plugin-eslint-comments: ^3.2.0 => 3.2.0 
    eslint-plugin-flowtype: ^5.1.3 => 5.10.0 (4.6.0)
    eslint-plugin-import: ^2.21.2 => 2.27.5 (2.20.1)
    eslint-plugin-jest: ^23.17.1 => 23.20.0 (26.9.0)
    eslint-plugin-node: ^11.1.0 => 11.1.0 
    eslint-plugin-promise: ^4.2.1 => 4.3.1 
    eslint-plugin-react: ^7.22.0 => 7.32.2 
    eslint-plugin-react-hooks: ^4.6.0 => 4.6.0 
    eslint-plugin-react-native: ^3.8.1 => 3.11.0 (4.0.0)
    eslint-plugin-standard: ^4.0.1 => 4.1.0 
    fake-indexeddb: ^4.0.1 => 4.0.2 
    get-yarn-workspaces: ^1.0.2 => 1.0.2 
    hermes-inspector-msggen:  1.0.0 
    husky: ^4.2.3 => 4.3.8 
    identity-obj-proxy: ^3.0.0 => 3.0.0 
    jest: ^29.5.0 => 29.5.0 
    jest-expo: ^48.0.2 => 48.0.2 
    launchdarkly-react-client-sdk: ^3.0.10 => 3.0.10 
    lerna: ^6.6.1 => 6.6.2 
    memo-parser:  undefined (0.2.1)
    patch-package: ^6.2.2 => 6.5.1 
    plop: ^2.7.4 => 2.7.6 
    plop-example:  undefined ()
    react: ^18.2.0 => 18.2.0 
    react-native: ^0.69.1 => 0.69.10 
    react-native-web: ^0.18.10 => 0.18.12 
    react-refresh: 0.11.0 => 0.11.0 (0.4.3)
    react-zoom-pan-pinch: ^1.6.1 => 1.6.1 
    ts-jest: ^29.0.5 => 29.1.0 
    ts-patch: ^3.1.1 => 3.1.1 
    typescript: 5.0.3 => 5.0.3 (5.0.4, 4.9.5, 4.7.4, )
    typescript-transform-paths: ^3.4.6 => 3.4.6 
    uuid: ^8.3.2 => 8.3.2 (3.4.0, 3.3.2, 7.0.3)
    yaml: ^1.10.0 => 1.10.2 
  npmGlobalPackages:
    corepack: 0.18.0
    git-cz: 4.9.0
    npm: 9.6.7
    yarn: 1.22.21


Describe the bug

Given that the Reachability class relies on navigator.onLine (which can indicate a false positive upon being connected to a network with no internet), we've noticed that the "ready" event of DataStore can take sometime to be triggered (as it remains trying to sync the models).
At the same time, since it's not aware that there's no connection, upon tryin to update a model, it uses an Exponential Backoff strategy and, when the client is actually connected, it might take some time (until the next try) to perform these updates.

Expected behavior

Based on the previously mentioned issue, we've had to handle on our app a heartbeat to verify the connection, but we've noticed that there's no way to manually trigger a DataStore.sync upon our app detecting that the connection is back but instead we need to wait for the next try of DataStore to dispatch those pending requests.

It would be nice to have that DataStore sync method exposed or maybe another way to feed the Reachability class (for now, we have it subscribe to a BroadcastChannel to receive a network status' msg).

Reproduction steps

  1. Open the app in a device connected to a network with no Internet.
  2. At this point, DataStore will keep trying to sync the models before the "ready" event (and can take a lot before it give up and fires it)
  3. The Reachability class will remain emitting false positives for the client's connection.
  4. Perform a DataStore action.
  5. At this point, it'll keep trying to send the new requests unsuccessfully.
  6. Connect the network back to the Internet.
  7. At this point, there can be a period in between pending requests' dispatches for DataStore to retry them.

Code Snippet

// Put your code below this line.

Log output

// Put your logs below this line


aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

@luisenaguero luisenaguero added the pending-triage Issue is pending triage label Apr 1, 2024
@cwomack cwomack added the DataStore Related to DataStore category label Apr 1, 2024
@cwomack cwomack self-assigned this Apr 1, 2024
@schutzelaars
Copy link

Our customers are experiencing the same issue where DataStore seems to be stuck syncing ('ready' not being emitted) on devices with very poor internet connections.

@luisenaguero could you share a code snippet of your workaround?

@luisenaguero
Copy link
Author

luisenaguero commented Apr 2, 2024

Hey, @schutzelaars !

What we ended up doing for now was to have the Reachability class to not only listen the online/offline events but also have it be subscribed to our own BroadcastChannel which receives a message based on a heartbeat we placed in a ServiceWorker that every so often makes a request to an url to verify the connectivity.

Reachability class

...
const broadcastChannel = new BroadcastChannel('CHANNEL_NAME');

return new Observable(observer => {
	observer.next({ online: globalObj.navigator.onLine });

	const notifyOnline = () => {
		observer.next({ online: true });
	};
	const notifyOffline = () => {
		observer.next({ online: false });
	};

        broadcastChannel.onmessage = (event) => {
           // checks the message
           // calls notifyOnline or notifyOffline accordingly 
        };

        return () => { 
           broadcastChannel.close();  
           ...

Service worker

...

const broadcastChannel = new BroadcastChannel('CHANNEL_NAME');

function simplifiedConnectivityChecker() {
  // pings url to verify
  broadcastChannel.postMessage(ONLINE/OFFLINE);
  ....
  // gap time
  simplifiedConnectivityChecker();
}
...

If the client has poor connection, either way this might not be completely accurate.
For this case we added like a timeout when pinging the url and if it takes longer than x secs, we considers it as if there's no connection.

Hope this helps.

@schutzelaars
Copy link

Thanks!

In the near future, I'll also implement a connectivity checker. Did you encounter any gotchas when implementing the simplifiedConnectivityChecker function?

@cwomack
Copy link
Contributor

cwomack commented Apr 3, 2024

Hello, @luisenaguero and thank you for opening this issue. I'm going to mark this as a feature request at this point after reviewing this with the team internally.

The purpose of the exponential backoff is to avoid inundating infrastructure that's attempting to come back online. That could be AppSync or any piece of "network" between the client and AppSync. Regardless of the network, it's a "best practice" to back off to give the "thing that's down" or unavailable room to breath and come back online.

That said, exponential backoff should inherently retry a handful of times very quickly. If the network is still connecting, I would expect reconnect within seconds and retry to also succeed within a few seconds. If the network is "down", I'd similarly expect exponential backoff to at least be proportional to the time to recovery.

@cwomack cwomack added feature-request Request a new feature and removed pending-triage Issue is pending triage labels Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataStore Related to DataStore category feature-request Request a new feature
Projects
None yet
Development

No branches or pull requests

4 participants