grpc / grpc-web

gRPC for Web Clients
https://grpc.io
Apache License 2.0
8.45k stars 761 forks source link

Retry interceptor #1366

Closed Nikjee closed 8 months ago

Nikjee commented 9 months ago

Hello i'm trying to implement retry interceptor based on error message but im not quite getting the idea. Looking for help or some examples would be event better. That's what i have so far.

class RefreshTokenInterceptor implements grpcWeb.UnaryInterceptor<any, any> {
  intercept(
    request: grpcWeb.Request<any, any>,
    invoker: (request: grpcWeb.Request<any, any>) => any
  ) {

    return invoker(request)
    .then(
      (response: grpcWeb.UnaryResponse<any, any>) => {
        return response;
      }
    )
    .catch((err) => {
        if(err.message.match('some kind of error'){
            ...do something
            ...do something with request meta

            return invoker(request)
        }

        return err
    })

    }

So it mostly works but when err is returned im getting this error in console TypeError: e.getResponseMessage is not a function

I assume i need to return UnaryResponse in catch block but error is instance of rpcError?

So i don't understand how to handle catch block and proceed next

sampajano commented 9 months ago

@Nikjee Thanks for the question and the code examples!

I gave this a quick try — i think if, instead of return err, you do:

throw err;

Then the error should go away. 😃


Another thing is, if you want this retry interceptor to work repeatedly (e.g. until a max numRetries), then you can consider adding an optional param to your intercept() method, like numRetries = 3, and instead of calling:

return invoker(request);

You could do something like:

return this.intercept(request, invoker, numRetries - 1);

Then you might have the interceptor work recursively, until it succeeds or a max number of retries have been reached.

Hope that helps :)


And yes, we do lack docs on interceptors (this is the best we have today), let me try to maybe add some docs around here 😃

UPDATE: I've added a little bit of docs on interceptors in the attached PR :)

Nikjee commented 9 months ago

@Nikjee Thanks for the question and the code examples!

I gave this a quick try — i think if, instead of return err, you do:

throw err;

Then the error should go away. 😃

Another thing is, if you want this retry interceptor to work repeatedly (e.g. until a max numRetries), then you can consider adding an optional param to your intercept() method, like numRetries = 3, and instead of calling:

return invoker(request);

You could do something like:

return this.intercept(request, invoker, numRetries - 1);

Then you might have the interceptor work recursively, until it succeeds or a max number of retries have been reached.

Hope that helps :)

And yes, we do lack docs on interceptors (this is the best we have today), let me try to maybe add some docs around here 😃

UPDATE: I've added a little bit of docs on interceptors in the attached PR :)

Yes this works, thanks for your help, but now i've got kinda strange behavior? Look at this example

Api.ts

list(){
    const req = new SomeKindListRequest()

    // This is a promise client that for example returns an error
    return client.list(req,metadata)
}
store.ts

//Getting error message in both blocks then and catch

listMethod(){
    api.list()
        .then(val => {
                //Here im getting error message tho it should be only in catch block
            })
         .catch(err => {
         //Here im getting error as it should be
         })

}

At first i thought it was a problem with interceptor but the following behavior still happens without it.

sampajano commented 9 months ago

@Nikjee Glad it worked (partially)!

Yes this works, thanks for your help, but now i've got kinda strange behavior? Look at this example

I'm sorry i'm not able to understand what's wrong without seeing all the code you have. Could you provide more complete context?

At first i thought it was a problem with interceptor but the following behavior still happens without it.

Are you saying that even without adding your custom interceptor, the issue is still happening? Could you describe the exact server behavior that triggers it?

Nikjee commented 9 months ago

@sampajano So after request is executed server sends a response with error message that should be handled on client in catch block, because it's an error, but in fact it gets passed to then and catch block at the same time, which is weird.

the example is simple

client.method(req,metadata).then(...).catch(...)

If server returns regular response it goes to then block as it should be, if it returns an error message, which should be in catch block, it gets passed in then block and catch. I wasn't developing backend of grpc server but i was told that nothing changed from other requests, so i guess the problem on my client side?

Btw is it possible to implement retry stream interceptor?

sorry for late response.

sampajano commented 8 months ago

@Nikjee Yes it's indeed weird that it's called on both callbacks.

Although It's difficult for me to understand what's going on without seeing your code.

Do you mind providing all your current code? (or put the repro in a github repro so i can test it out?)

Thanks alot!

Nikjee commented 8 months ago

@Nikjee Yes it's indeed weird that it's called on both callbacks.

Although It's difficult for me to understand what's going on without seeing your code.

Do you mind providing all your current code? (or put the repro in a github repro so i can test it out?)

Thanks alot!

@sampajano Uh i can't really do that since it's a project from work and is on private gitlab, but i think the problem should be on backend side since all my grpc services on client side are the same

sampajano commented 8 months ago

@sampajano Uh i can't really do that since it's a project from work and is on private gitlab, but i think the problem should be on backend side since all my grpc services on client side are the same

@Nikjee Totally understandable..

I didn't mean your production code.. but a small snippet representing how you implemented the interceptor (i.e. a minimal repro case).

In general, unless your backend is behaving badly, i'd still be surprised to hear both response and error callbacks be invoked.

But it's your call whether to proceed here. If you do not need any assistance at the moment, feel free to close the issue.

thanks! :)

Nikjee commented 8 months ago

@sampajano Uh i can't really do that since it's a project from work and is on private gitlab, but i think the problem should be on backend side since all my grpc services on client side are the same

@Nikjee Totally understandable..

I didn't mean your production code.. but a small snippet representing how you implemented the interceptor (i.e. a minimal repro case).

In general, unless your backend is behaving badly, i'd still be surprised to hear both response and error callbacks be invoked.

But it's your call whether to proceed here. If you do not need any assistance at the moment, feel free to close the issue.

thanks! :)

Ah. i misunderstood you, here is full interceptor code

import { AuthApi } from './user/authApi';
import * as grpcWeb from 'grpc-web';
import { Notify } from 'quasar';

interface UnaryInterceptor<REQ, RESP> {
  intercept(
    request: grpcWeb.Request<REQ, RESP>,
    invoker: (
      request: grpcWeb.Request<REQ, RESP>
    ) => Promise<grpcWeb.UnaryResponse<REQ, RESP>>,
    numRetries: number
  ): Promise<grpcWeb.UnaryResponse<REQ, RESP>>;
}

const authApi = new AuthApi();

const errMsgTokenSegments =
  'access token is invalid: invalid token: token contains an invalid number of segments';

const errMsgTokenInvalid = 'access token is invalid';

class RefreshTokenInterceptor implements UnaryInterceptor<any, any> {
  intercept(
    request: grpcWeb.Request<any, any>,
    invoker: (request: grpcWeb.Request<any, any>) => any,
    numRetries = 2
  ) {
    return invoker(request)
      .then((response: grpcWeb.UnaryResponse<any, any>) => {
        return response;
      })

      .catch(async (err) => {

        if (numRetries == 1) throw err;

        if (err.message.match('refresh token in undefined')?.[0]) {
          Notify.create({
            message: 'Session is expired.',
            type: 'negative',
          });

          throw err;
        }

        if (err.message.match(errMsgTokenSegments)?.[0]) {
          Notify.create({
            message: 'Session expired',
            type: 'negative',
          });
          throw err;
        }

        if (err.message.match(errMsgTokenInvalid)?.[0]) {
          const refreshKey = localStorage.getItem('refresh-key');
          if (refreshKey) {
            return await authApi
              .RefreshToken({
                refreshToken: refreshKey,
              })
              .then((res) => {
                request.getMetadata().Authorization = res.getAccessToken();
                return this.intercept(request, invoker, numRetries - 1);
              });
          }
        }

        throw err;
      });
  }
}

export const RefreshTokenMiddleware = RefreshTokenInterceptor;

But overall this behavior still happens without interceptor, and also i checked backend side and its implementation is the same as in other methods

EDIT: I think i recognized where the problem is, it is indeed on backend side but in its interceptor, tho i can see it was made like in guide interceptor for go and it should be working properly but my guess is that interceptor returns error into response and error accordingly...

This was used as a guide for interceptor and pretty much the same

sampajano commented 8 months ago

Thanks for providing the full code! It's very helpful for understanding what you're trying to achieve!

But overall this behavior still happens without interceptor, and also i checked backend side and its implementation is the same as in other methods

Are you saying that with or without an interceptor on grpc-web side, the same behavior is happening?

I guess that rules out this being an issue with grpc-web interceptor, right?

EDIT: I think i recognized where the problem is, it is indeed on backend side but in its interceptor, tho i can see it was made like in guide interceptor for go and it should be working properly but my guess is that interceptor returns error into response and error accordingly...

Aha got it! Interesting.. I'm not familiar with grpc go, but it's odd that you'd have this behavior when following the user guide..

I'm wondering what response are you getting from the server. It would help us understand exactly whether the issue is with server or client.

I haven't used this tool myself, but i guess it could help you understand what response was being returned from server.

Nikjee commented 8 months ago

Thanks for providing the full code! It's very helpful for understanding what you're trying to achieve!

But overall this behavior still happens without interceptor, and also i checked backend side and its implementation is the same as in other methods

Are you saying that with or without an interceptor on grpc-web side, the same behavior is happening?

I guess that rules out this being an issue with grpc-web interceptor, right?

EDIT: I think i recognized where the problem is, it is indeed on backend side but in its interceptor, tho i can see it was made like in guide interceptor for go and it should be working properly but my guess is that interceptor returns error into response and error accordingly...

Aha got it! Interesting.. I'm not familiar with grpc go, but it's odd that you'd have this behavior when following the user guide..

I'm wondering what response are you getting from the server. It would help us understand exactly whether the issue is with server or client.

I haven't used this tool myself, but i guess it could help you understand what response was being returned from server.

Yea i've been using it, it shows that the request received error but how .then callback gets executed?

So this scenario works fine

File A

export someKindFunc() {
return client.request(req,metadata)
}
File B
import someKindFunc

someKindFunc().then(...).catch(error)

But if i do this,it wont

File A

export someKindFunc() {
return client.request(req,metadata).catch(err => {
    console.log(err)
    return err
}
File B
import someKindFunc

someKindFunc().then(this gets error).catch(and this logs error if somekind error happend in then cb)

Maybe im not understanding properly how Promises work huh? If you return something from catch block it will be passed down the chain to then cb?

image

sampajano commented 8 months ago

@Nikjee Hi thanks for providing the details!

Seeing that you've close the bug. I'm hoping the issue is now resolved? 😃

If you could provide a very brief summary on what worked for you, it might help everyone having the same issue in the future.

Thanks! 😃