Add abortSignal to unaryCall

[benvernier-sc]

The goal of this PR is to add an abortSignal parameter when making calls using the PromiseClient, which cancels the gRPC stream when the signal is aborted.

There has already been interest in being able to cancel calls made using promises: https://github.com/grpc/grpc-web/issues/946

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: benvernier-sc / name: Ben Vernier (d9fdd44000b2d2b3b3e331b36569225c8d8d6e41)

[sampajano]

Hi! Thanks a lot for the interests here and the PR!

A few quick thoughts.. :)

1. Abort signal seems like a generally useful feature to have and would be a nice addition.
2. This is a fairly major change to the top-level API, and would require a larger design review in my team (beyond myself) before i can respond to you.
3. (credit to @stanley-cheung) It's probably not very sustainable if we keep adding parameters for new control we need it a future. We should probably create a new "option" bundle where we can have users specify this as an option (and more extensible for more in the future).
4. And if we were to make this change, we should probably make it to rpcCall and serverStreaming too, not just thenableCall.
5. (credit to @stanley-cheung) This change could also be "breaking" for users who upgraded their code-gen binary but not the JS version (in a perfect world people would always upgrade both at the same time but it's often not the case), so it requires extra caution.
   1. I also need to experiment with this change on Google codebase too to confirm that.
6. More tests / docs should probably be added for this feature.

Just a few quick thoughts before i forget.. but there are maybe more.. :)

Thanks so much for your contrib here again! 😃

(UPDATED with inputs from @stanley-cheung)

[dbssAlan]

Hello, what's the progress?😊

[sampajano]

@dbssAlan hi thanks for checking!

i think we won't take this PR as is, due to the API change requiring internal validations.

Rather, we will:

1. Implement and experiment this feature in Google internal first.
2. Finalize the API change.
3. Update this PR per the finalized API, then merge it.

So, it'll be pending on our side to do Step 1 first. Thanks!

[iampava]

Hey! Any updates on this?

If not, I was wondering what patterns you use to cancel grpc calls via the Promise client.

[dbssAlan]

Hey! Any updates on this?

If not, I was wondering what patterns you use to cancel grpc calls via the Promise client.

I think that we can use Promise.all or Promise.race to stop async/await from blocking execution in JS.

[sampajano]

Hey! Any updates on this?

If not, I was wondering what patterns you use to cancel grpc calls via the Promise client.

Hi! I'll try to find some time to experiment with this in the coming weeks. thanks!

(i.e. Implement and experiment this feature in Google internal first. as mentioned above)

[iampava]

I managed to "hack it" by using "AbortController" and verifying if an abort signal has been sent.

[sampajano]

I managed to "hack it" by using "AbortController" and verifying if an abort signal has been sent.

Just curious how exactly did you manage to "hack it"? 😃

If not, I was wondering what patterns you use to cancel grpc calls via the Promise client.

Before thie PR i don't think it's possible to cancel an ongoing grpc call so far..

[iampava]

I managed to "hack it" by using "AbortController" and verifying if an abort signal has been sent.

Just curious how exactly did you manage to "hack it"? 😃

If not, I was wondering what patterns you use to cancel grpc calls via the Promise client.

Before thie PR i don't think it's possible to cancel an ongoing grpc call so far..

You're right. What I meant by "hacked it" was that I added additional code in my services that double checks that request was meant to be cancelled, before doing any side effects. So I'm not actually cancelling it for real.

Something like:

async function getUser(userId, abortController) {
   const getUserRequest = new GetUserRequest(userId);
   const getUserResponse = await UserService.getUser(getUserRequest);

  if (abortController.signal.aborted) {
    // ...
  }

[sampajano]

@iampava aha i see! Thanks for clarifying! 😃

In this case, i'm supposing that you don't have a way to pass the abortController to grpc-web right (since our API doesn't take it right now)? So i guess it wouldn't work even if abort() is called..

[iampava]

@sampajano correct. I have to admit, it would be nice if grpc-web would accept an AbortController - this would make it on par with the native Fetch API, and people might have an easier time understanding how to cancel things.

For me, this was my first thought: let me see if grpc-web accepts an AbortController. Oh, looks like it doesn't...

[sampajano]

@iampava Yup! makes sense! Definitely would be a good addition.. As mentioned above, i'll try to experiment with this feature internally first soon, and upstream it to GH if it works. Thanks for the interest here!

[sampajano]

In the meanwhile, I'll close this PR for now since this probably won't be the API we'll adopt (we'll probably try to make it more generic and extensible, in case we need to pass more similar options later). Thanks for the contrib!

[dmitryshelomanov]

@sampajano have any progress ? Can I abort unary call ?

export class Client extends GrpcWebImpl {
  unary<T extends UnaryMethodDefinitionish>(
    methodDesc: T,
    _request: any,
    metadata: grpc.Metadata | undefined,
  ): Promise<any> {
    const request = { ..._request, ...methodDesc.requestType };
    const maybeCombinedMetadata =
      metadata && this.options.metadata
        ? new BrowserHeaders({ ...this.options?.metadata.headersMap, ...metadata?.headersMap })
        : metadata || this.options.metadata;
    let req: grpc.Request | null = null;
    const deffer: any = {};

    const promise = new Promise((resolve, reject) => {
      deffer.resolve = resolve;
      deffer.reject = reject;

      req = grpc.unary(methodDesc, {
        request,
        host: this.host,
        metadata: maybeCombinedMetadata,
        transport: this.options.transport,
        debug: this.options.debug,
        onEnd: response => {
          if (response.status === grpc.Code.OK) {
            resolve(response.message!.toObject());
          } else {
            const err = new GrpcWebError(response.statusMessage, response.status, response.trailers);
            reject(err);
          }
        },
      });
    });

    // @ts-ignore
    promise.abort = () => {
      req?.close();
      deffer?.reject(
        new GrpcWebError(
          'CANCELLED',
          grpc.Code.Canceled,
          new BrowserHeaders({ ...(this.options?.metadata?.headersMap ?? {}), ...metadata?.headersMap }),
        ),
      );
    };

    return promise;
  }
}

I tried to create own class extends web grpc code And make unary update

close method from request dont send error when call req.cancel()

[sampajano]

@dmitryshelomanov Hi thanks for checking. This is planned for the upcoming quarter so hopefully i will provide an update soon!

[maja42]

Any updates on this?

[sampajano]

@maja42 Hi thanks for checking!

I intended to do this but was not able to prioritize it yet..

i'll again bump this in my list and i hope to get to it in the upcoming weeks!