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

API.cancel doesn't work #9198

Closed
3 tasks done
gweisz-440it opened this issue Nov 11, 2021 · 10 comments · Fixed by #9578
Closed
3 tasks done

API.cancel doesn't work #9198

gweisz-440it opened this issue Nov 11, 2021 · 10 comments · Fixed by #9578
Assignees
Labels
API Related to REST API issues bug Something isn't working

Comments

@gweisz-440it
Copy link

Before opening, please confirm:

JavaScript Framework

React Native

Amplify APIs

GraphQL API

Amplify Categories

api

Environment information

System:
    OS: Windows 10 10.0.22000
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
    Memory: 2.16 GB / 15.69 GB
  Binaries:
    Node: 14.18.0 - C:\Program Files\nodejs\node.EXE
    npm: 6.14.15 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.22000.120.0), Chromium (95.0.1020.44)
    Internet Explorer: 11.0.22000.120
  npmPackages:
    @aws-amplify/ui-components: ^1.7.2 => 1.7.7
    @babel/core: ^7.15.0 => 7.15.5
    @babel/plugin-transform-modules-commonjs: ^7.15.0 => 7.15.4
    @babel/preset-env: ^7.15.0 => 7.15.4
    @babel/preset-react: ^7.14.5 => 7.14.5
    @babel/preset-typescript: ^7.15.0 => 7.15.0
    @babel/runtime: ^7.15.3 => 7.15.4
    @react-native-async-storage/async-storage: ^1.15.5 => 1.15.7
    @react-native-community/eslint-config: ^2.0.0 => 2.0.0
    @react-native-community/netinfo: ^6.0.0 => 6.0.2
    @react-native-firebase/app: ^12.4.0 => 12.7.5
    @react-native-firebase/crashlytics: ^12.4.0 => 12.7.5
    @react-native-masked-view/masked-view: ^0.2.6 => 0.2.6
    @react-navigation/drawer: ^6.1.0 => 6.1.4
    @react-navigation/native: ^6.0.2 => 6.0.2
    @react-navigation/native-stack: ^6.1.0 => 6.1.0
    @react-navigation/stack: ^6.0.3 => 6.0.7
    @reduxjs/toolkit: ^1.6.1 => 1.6.1
    @reduxjs/toolkit-query:  1.0.0
    @reduxjs/toolkit-query-react:  1.0.0
    @testing-library/jest-native: ^4.0.2 => 4.0.2
    @testing-library/react-native: ^7.2.0 => 7.2.0
    @types/jest: ^26.0.14 => 26.0.24
    @types/react: ^17.0.19 => 17.0.20
    @types/react-native: ^0.64.13 => 0.64.15 (0.65.0)
    @types/react-native-honeywell-scanner: ^1.0.0 => 1.0.0
    @types/react-test-renderer: ^17.0.1 => 17.0.1
    @typescript-eslint/eslint-plugin: ^4.29.3 => 4.31.0 (3.10.1)
    @typescript-eslint/parser: ^4.29.3 => 4.31.0 (3.10.1)
    HelloWorld:  0.0.1
    amazon-cognito-identity-js: ^5.0.6 => 5.1.0
    amplify-ui-components-loader:  undefined ()
    aws-amplify: ^4.2.2 => 4.2.8
    babel-jest: ^26.3.0 => 26.6.3
    eslint: ^7.32.0 => 7.32.0
    hermes-inspector-msggen:  1.0.0
    husky: ^7.0.0 => 7.0.2
    i18next: ^20.4.0 => 20.6.1
    jest: ^26.4.2 => 26.6.3
    metro-react-native-babel-preset: ^0.64.0 => 0.64.0
    native-base: ^3.0.7 => 3.1.0
    react: 17.0.1 => 17.0.1
    react-hook-form: ^7.15.2 => 7.15.2
    react-i18next: ^11.11.4 => 11.11.4
    react-native: 0.64.1 => 0.64.1
    react-native-code-push: ^7.0.1 => 7.0.1
    react-native-config: ^1.4.3 => 1.4.4
    react-native-elements: ^3.4.2 => 3.4.2
    react-native-exception-handler: ^2.10.10 => 2.10.10
    react-native-fs: ^2.18.0 => 2.18.0
    react-native-gesture-handler: ^1.10.3 => 1.10.3
    react-native-honeywell-scanner: ^2.0.0 => 2.0.0
    react-native-image-picker: ^4.0.6 => 4.0.6
    react-native-reanimated: ^2.2.0 => 2.2.0
    styled-components/native:  undefined ()
    styled-components/primitives:  undefined ()
    styled-system: ^5.1.5 => 5.1.5
    ts-jest: ^26.4.1 => 26.5.6
    typescript: ^3.9.10 => 3.9.10
  npmGlobalPackages:
    react-native-cli: 2.0.1

Describe the bug

We are trying to standardize an app behavior under poor connection scenarios, what we'd like to achieve is to cancel pending GraphQL requests if these don't fulfill under a given timeframe.

For that purpose we've already implemented the time related logic, but when we try to cancel the pending requests with the API.cancel as described in https://docs.amplify.aws/lib/graphqlapi/cancel-request/q/platform/js/ it does nothing

We have also tried to get help in the discord channels but without any response, other ppl seem to have incurred into the same issue

We've taken care of using the original promise (not a wrapped one) to cancel the request as the documentation warns

Expected behavior

We expect that when calling API.cancel the request is immediately cancelled

Reproduction steps

To easily explain what we are expecting to happen, a 'simple example' is provided as a code snippet, in this example, and based on what we see in the documentation, we expect an error to be thrown, but instead the request is completed normally

Code Snippet

// Put your code below this line.
console.log(new Date().toISOString(), 'before api.graphql')

const promise = API.graphql({
  query: ...,
  variables: ...
})

console.log(new Date().toISOString(), 'before api cancel')

API.cancel(promise, 'test')

console.log(new Date().toISOString(), 'after api cancel')

const result = (await promise)

console.log(new Date().toISOString(), 'result: ', result)

...
} catch (error: unknown) {
  console.log(new Date().toISOString(), 'an error occurred: ', error)
  ...
}

Log output

// Put your logs below this line
 LOG  2021-11-11T17:58:22.179Z before api.graphql
 LOG  2021-11-11T17:58:22.188Z before api cancel
 LOG  2021-11-11T17:58:22.189Z after api cancel
 LOG  2021-11-11T17:58:24.337Z result:  {...}

 LOG  2021-11-11T17:58:41.833Z before api.graphql
 LOG  2021-11-11T17:58:41.835Z before api cancel
 LOG  2021-11-11T17:58:41.837Z after api cancel
 LOG  2021-11-11T17:58:42.735Z result:  {...}

 LOG  2021-11-11T17:58:54.768Z before api.graphql
 LOG  2021-11-11T17:58:54.771Z before api cancel
 LOG  2021-11-11T17:58:54.773Z after api cancel
 LOG  2021-11-11T17:58:55.652Z result:  {...}

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

@david-mcafee
Copy link
Contributor

Hi @gweisz-440it - perhaps you haven't included it in your sample, but have you tried if(API.isCancel(error)) in your try / catch block, as outlined in the docs?

@chrisbonifacio chrisbonifacio added API Related to REST API issues investigating This issue is being investigated labels Nov 11, 2021
@chrisbonifacio chrisbonifacio added pending-response and removed investigating This issue is being investigated labels Nov 11, 2021
@gweisz-440it
Copy link
Author

Hey @david-mcafee in the example isCancel receives an error, I have no error, since the api.graphql promise resolves correctly after some time (even though it was cancelled), please see the log times

@chrisbonifacio chrisbonifacio added the investigating This issue is being investigated label Nov 12, 2021
@cowjen01
Copy link

cowjen01 commented Feb 7, 2022

Hello, I'm facing the same issue. Even if I cancel the request immediately, nothing happens and no errors are thrown.

@jowo-io
Copy link

jowo-io commented Feb 11, 2022

also have the same problem

@jowo-io
Copy link

jowo-io commented Feb 11, 2022

I tried debugging the issue by adding a console log to the amplify code:

    RestClient.prototype.cancel = function (request, message) {
        var source = this._cancelTokenMap.get(request);
        console.log("source", source); // <-- Added this
        if (source) {
            source.cancel(message);
        }
        return true;
    };

and source returns as undefined, so obviously the cancel method is never invoked. Trying to figure out why...

@jowo-io
Copy link

jowo-io commented Feb 11, 2022

I've done a little more digging, and it seems that the this._cancelTokenMap WeakMap contains no entries. So when the original promise is provided to the WeakMap's get method, it obviously can't find anything because the WeakMap itself contains nothing. Not sure why the Map is empty.
Investigating a bit further...

@jowo-io
Copy link

jowo-io commented Feb 11, 2022

It seems like the RestClient is being initialised multiple times under the hood of amplify, and each time it creates a new WeakMap. Can't figure out much more than that sadly, not sure why RestClient is being initialised multiple times.

@jamesaucode
Copy link
Contributor

@jowo-io You are on the right track looking at the _cancelTokenMap. I think the issue is that API.cancel actually only calls the .cancel on ._restAPI, see:

/**
* Cancels an inflight request
* @param {any} request - request to cancel
* @return {boolean} - A boolean indicating if the request was cancelled
*/
cancel(request: Promise<any>, message?: string) {
return this._restApi.cancel(request, message);
}

I believe changing line 162 to

return this._restApi.cancel(request, message) || this._graphqlApi.cancel(request, message);

should fix this issue
I will do some testing locally to verify it and get a PR out soon

@jamesaucode jamesaucode added bug Something isn't working and removed investigating This issue is being investigated labels Feb 11, 2022
@jowo-io
Copy link

jowo-io commented Feb 14, 2022

@jamesaucode ahh, i see! that's brilliant though thanks for opening the PR on this issue.

@chrisbonifacio chrisbonifacio changed the title API.cancel doesn't works API.cancel doesn't work Apr 19, 2022
@github-actions
Copy link

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server amplify-help forum.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Related to REST API issues bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants