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

Add QueuedInterceptor instead of interceptors.lock #1308

Closed
wendux opened this issue Oct 31, 2021 · 42 comments
Closed

Add QueuedInterceptor instead of interceptors.lock #1308

wendux opened this issue Oct 31, 2021 · 42 comments
Labels
s: feature This issue indicates a feature request

Comments

@wendux
Copy link
Contributor

wendux commented Oct 31, 2021

What is QueuedInterceptor?

As we all know, we can intercept requests/responses/errors before they are handled by then or catchError with
dio Interceptor。In previous versions, interceptors could be executed concurrently, that is, all of the requests enter the interceptor at once, rather than executing sequentially. However, in some cases we expect that requests enter the interceptor sequentially like #590 。 Therefore, we need to provide a mechanism for sequential access(one by one) to interceptors and QueuedInterceptor is proposed to solve this problem.

Examples

We make 3 concurrent requests, all of which enter the interceptor at the same time when using Interceptor; But when using QueuedInterceptor , requests enter the interceptor sequentially.

void main() async {
  var dio = Dio();
  dio.options.baseUrl = 'http://httpbin.org/status/';
  dio.interceptors.add(
    InterceptorsWrapper(
      onRequest: (
        RequestOptions requestOptions,
        RequestInterceptorHandler handler,
      ) {
        print(requestOptions.uri);
        Future.delayed(Duration(seconds: 2), () {
          handler.next(requestOptions);
        });
      },
    ),
  );
  print( 'All of the requests enter the interceptor at once, rather than executing sequentially.');
  await makeRequests(dio);

  print( 'All of the requests enter the interceptor sequentially by QueuedInterceptors');
  dio.interceptors
    ..clear()
    ..add(
      // [QueuedInterceptorsWrapper] is a helper class, which is used to conveniently create QueuedInterceptor.
      QueuedInterceptorsWrapper( 
        onRequest: (
          RequestOptions requestOptions,
          RequestInterceptorHandler handler,
        ) {
          print(requestOptions.uri);
          Future.delayed(Duration(seconds: 2), () {
            handler.next(requestOptions);
          });
        },
      ),
    );
  await makeRequests(dio);
}

Future makeRequests(Dio dio) async {
  try {
    await Future.wait([
      dio.get('/200'),
      dio.get('/201'),
      dio.get('/201'),
    ]);
  } catch (e) {
    print(e);
  }
}

Of course, you can implement your own queued interceptor directly by inheriting from QueuedInterceptor.

Delete Locks of interceptors

Locks of interceptors were originally designed to synchronize interceptor execution, but locks have a problem that once it becomes unlocked all of the requests run at once, rather than executing sequentially. Now QueuedInterceptor can do it better.

To delete Lock another reason is that Lock is shared between the interceptors, so Lock can be called by any interceptors at any place, it will bring potential risks, assuming a interceptor invokes lock, but because the code defects, lead to can't normal call unLock, this will affect the rest of the interceptor to normal work (may never be executed to).

How to try QueuedInterceptor.

Use 4.0.2-beta1 or change to develop branch.

@wendux wendux added the s: feature This issue indicates a feature request label Oct 31, 2021
@wendux wendux added the merged label Nov 14, 2021
@akifarhan
Copy link

It works for me! 💯

@AlexanderFarkas
Copy link

AlexanderFarkas commented Nov 29, 2021

Why do you deprecate locks? Could you provide example of token refreshment? In that case you need lock only for one part of the interceptor. (In the case where you repeat request)

@comatory
Copy link

Could you provide migration guide for requestLock and responseLock. It's not really clear to me how to replace it.

@RumoneAnderson
Copy link

RumoneAnderson commented Dec 14, 2021

requestLock and responseLock can be removed. They were used to prevent concurrent interceptors which is now handled by QueuedInterceptor

@comatory
Copy link

comatory commented Dec 14, 2021

@RumoneAnderson So by simply removing and replacing InterceptorWrapper with QueuedInterceptorsWrapper it's guaranteed the behaviour will be the same?

@RumoneAnderson
Copy link

RumoneAnderson commented Dec 15, 2021

Being a guarantee that it will work for you, not sure. However, I have tested it and it works as expected.

@orteney
Copy link

orteney commented Dec 18, 2021

How to clear a QueuedInterceptor's queue for cases when auth token unable to be refreshed?
in 4.0.4 dio.clear() method are only clears lock's queue

@aboutandre
Copy link

We use the locks to deal with expired tokens and I'm not too comfortable changing this without good documentation.

Could someone please provide a concrete migration example for this?

@fullflash
Copy link

fullflash commented Dec 20, 2021

How to clear a QueuedInterceptor's queue for cases when auth token unable to be refreshed? in 4.0.4 dio.clear() method are only clears lock's queue

this case cannot be handled currently with QueuedInterceptor. before with could clear unlock QueuedInterceptor holds ques internally no way to reset.

after token refresh failed interceptor becames inactive no error or result and we getting loading with no result.

@mosi2142
Copy link

mosi2142 commented Dec 24, 2021

with this new changes. what is the solution for refreshing tokens ?

@wendux
Copy link
Contributor Author

wendux commented Dec 27, 2021

Why do you deprecate locks? Could you provide example of token refreshment? In that case you need lock only for one part of the interceptor. (In the case where you repeat request)

A example for refreshing tokens with QueuedInterceptor : https://github.com/flutterchina/dio/blob/develop/example/lib/queued_interceptor_crsftoken.dart

@TercyoStorck
Copy link

How do I lock/unlock the requests?

@w568w
Copy link

w568w commented Jan 20, 2022

It looks not so good to me, in my scenario.

I am using requestLock in my project for temporarily locking a dio instance and using another instance (with different Cookie jars, so they have to be held separately) to request a token for the former.

Since the token could be expired at any time, I have to check every response of the first dio and decide whether it is necessary to get a new token and replay the request. In that case, with requestLock I can simply lock the first, request a new token and unlock it.

But it isn't so easy to me when using a QueuedInterceptor. When should the requests of the first dio be blocked? It seems much harder to implement the function.

@comatory
Copy link

I am using requestLock in my project for temporarily locking a dio instance and using another instance (with different Cookie jars, so they have to be held separately) to request a token for the former one.

I'd like to know this as well. My code contains multiple dio instances, some of them are set up differently. At the moment I can decide which ones to lock/unlock etc.

@TimurMukhortov
Copy link

Is sample auth interceptor https://gist.github.com/TimurMukhortov/a1c9819e3779015e54bc3964b7d2308a for dio. its work for me.

@ipcjs
Copy link
Contributor

ipcjs commented Mar 1, 2022

@wendux

l. My code contains multiple dio instances, some of them are set up differently. At the moment I can decide which ones to lock/unlock etc.

There is a problem with this implementation code, if an error is reported when dio.fetch() is called in onError, the error will not be passed to the outside.
Here is sample code: ipcjs#1

@arafaysaleem
Copy link

I don't understand the need to deprecate the .clear() method. There is a chance someone might want to clean the waiting queue even if they are using QueuedInterceptor implementation.

For e.g. There might be repeated duplicate requests to the same endpoint, while the token is being refreshed. All of them get inserted in the queue, ready to run after the token is refreshed. However, after it is done, I might only want to execute the original request once, with the new token, and remove all the other duplicate requests. How is it possible to achieve this without clearing the queue?

@fullflash
Copy link

fullflash commented Apr 3, 2022

@wendux

l. My code contains multiple dio instances, some of them are set up differently. At the moment I can decide which ones to lock/unlock etc.

There is a problem with this implementation code, if an error is reported when dio.fetch() is called in onError, the error will not be passed to the outside. Here is sample code: ipcjs#1

using with legacy it work properly but with QueuedInterceptorsWrapper not.

the problem is at line 428 if (!taskQueue.processing) { return true but taskQueue.queue is empty it get stucks.
Screen Shot 2022-04-04 at 01 33 26

@ipcjs
Copy link
Contributor

ipcjs commented Apr 3, 2022

@fullfash I have create a PR(#1457 ) to fix the queued_interceptor_crsftoken bug.

The key point to note about using QueuedInterceptor is that the current dio instance is already blocked and another dio instance needs to be used to execute the request.

@fullflash
Copy link

@ipcjs but same situation can be happen on second dio instance also.

@ipcjs
Copy link
Contributor

ipcjs commented Apr 3, 2022

Don't add a QueuedInterceptor to the second dio instance, so it won't be blocked. 😅

@fullflash
Copy link

fullflash commented Apr 3, 2022

yes but wondering if there can be any leaking case. 🤔
on repeat request there might be also a token expiration very small chance but...
there must be an error prone fix for QueuedInterceptor logic.

anyway thanks . we will use tokenDio for repeat request also

@arafaysaleem
Copy link

I don't understand the need to deprecate the .clear() method. There is a chance someone might want to clean the waiting queue even if they are using QueuedInterceptor implementation.

For e.g. There might be repeated duplicate requests to the same endpoint, while the token is being refreshed. All of them get inserted in the queue, ready to run after the token is refreshed. However, after it is done, I might only want to execute the original request once, with the new token, and remove all the other duplicate requests. How is it possible to achieve this without clearing the queue?

@wendux @ipcjs can somebody clear this for me?

@ipcjs
Copy link
Contributor

ipcjs commented Apr 3, 2022

@fullflash you are right. maybe we need add a repeatDio, like this: ipcjs@dfe102d
🤔️

@fullflash
Copy link

@ipcjs this is a good workaround.
but i am still trying find a fix for QueuedInterceptor. there should be url mapping for overriding duplicate url request queues
and that can fix the stuck on repeated dio calls.

@fullflash
Copy link

@ipcjs
our repeated request error bug is about QueuedInterceptors error handler.

i came we this patch: to keep active processing que and force complete when same request path and data repeats.

if (taskQueue.activeQueue != null &&
        taskQueue.processing &&
        handler is ErrorInterceptorHandler) {
      final DioError cur = data as DioError;
      final DioError a = taskQueue.activeQueue!.data! as DioError;
      if (a.requestOptions.path == cur.requestOptions.path &&
          a.requestOptions.data == cur.requestOptions.data) {
        // taskQueue.processing = false;
        final ErrorInterceptorHandler activeErrHandler =
            taskQueue.activeQueue!.handler;
        activeErrHandler.next(taskQueue.activeQueue!.data);
      }
    }

this bypasses your test.
Will use such hardcoded way of dio package and make some tests to be sure.

@ipcjs
Copy link
Contributor

ipcjs commented Apr 4, 2022

To modify the lib you need to ask the maintainer of the project. @wendux @kuhnroyal
I think it is better to modify the example.

@iamchathu
Copy link

How to clear the queue? I got refreshing token happening 5 times to refresh the token. Is there a way to clear the queue?

@antoinepemeja
Copy link
Contributor

antoinepemeja commented Jun 15, 2022

Thank @ipcjs for your suggestion.

About this sample https://github.com/flutterchina/dio/blob/dfe102d5bed4da72e8f77f8f67395ba2b7d47b40/example/lib/queued_interceptor_crsftoken.dart

If you use another Dio instance to repeat the request (line 107 and line 83), everything should be working.

For this new Dio instance, don't forget to add your QueuedInterceptorsWrapper interceptor too.

Dio library does not seem to be maintained anymore ... does it?

@fullflash
Copy link

fullflash commented Jun 17, 2022

Thank @ipcjs for your suggestion.

About this sample https://github.com/flutterchina/dio/blob/dfe102d5bed4da72e8f77f8f67395ba2b7d47b40/example/lib/queued_interceptor_crsftoken.dart

If you use another Dio instance to repeat the request (line 107 and line 83), everything should be working.

For this new Dio instance, don't forget to add your QueuedInterceptorsWrapper interceptor too.

Dio library does not seem to be maintained anymore ... does it?

unfortunately it looks abandoned.
need alternative for "no hello world" applications )

Zead Is Dead Baby
https://www.youtube.com/watch?v=zgGFK5drCpk

@boaz23
Copy link

boaz23 commented Jun 18, 2022

Can an explanation be provided for why locks have become deprecated? I'm all for adding new features users want, but why remove the other one? As I see it, those are vastly different alternatives with pros and cons of their own. So the user should have the freedom of choice.

I've recently started implementing the token refresh logic in my application, and was glad to see the locking mechanism. But then, I was saddened to see it became deprecated, without an explanation on why.
I've looked in the #590 thread and this thread for such an explanation, but haven't found one.
If there is an such an explanation, I would be glad to read it.

While I agree that the logic is more complex using the locking mechanism than using the queue mechanism, but I think it should be kept for the users who want to use it.
I think I have an idea on for the token refresh logic while using the locking mechanism.

@ahmedtanjim
Copy link

What is the alternative of requestLock

@ChiPhanTlm
Copy link

same here, any solution? create new instance is not a good practice!

@kostadin-damyanov-prime

Using this interceptor prevents you from being able to make parralel calls (because it serialises them)?
Did I get this right?

How is this better than the lock/unlock technique which blocks the queue ONLY when a token refresh is in progress (for example)?

@kuhnroyal
Copy link
Member

Only QueuedInterceptors are serial, all other interceptors and the request are parallel.
If your QueuedInterceptor does not await anything on the happy path, this will have practically no noticeable effect.

Very simplified it looks something like the following, only when isTokenValid == false, the following request are blocked and will wait until the token refresh is finished.

bool isTokenValid = true;

QueuedInterceptorsWrapper( 
        onRequest: (
          RequestOptions requestOptions,
          RequestInterceptorHandler handler,
        ) {
          if (!isTokenValid) {
			await refreshToken();
			isTokenValid = true;
		  } 
          handler.next(requestOptions);
        },
);

@timnew
Copy link

timnew commented Dec 22, 2022

@wendux Would mind to provide more clarification on how should retry being done from onError.

As I found the solution in example code that calling dio.fetch on the same instance in onError would cause deadlock.

Wondering how it should be done properly: checkout issue #1612

@boaz23
Copy link

boaz23 commented Dec 26, 2022

@kuhnroyal Interesting, thank you. I was wrong then.

@AbdurRehman-coder
Copy link

Thanks, Queued Interceptors solve my problems, now I can handle my refresh and access token very efficiently. The best part of Queued Interceptors is this, it retries automatically the failed requests.

@logispire
Copy link

Why do you deprecate locks? Could you provide example of token refreshment? In that case you need lock only for one part of the interceptor. (In the case where you repeat request)

A example for refreshing tokens with QueuedInterceptor : https://github.com/flutterchina/dio/blob/develop/example/lib/queued_interceptor_crsftoken.dart

The perfect solution with example, thanks a lot, you saved my day !!

@stngcoding
Copy link

does anyone have working example? I cant get the refreshToken to run only once
` @OverRide
void onRequest(
RequestOptions options,
RequestInterceptorHandler handler,
) async {
final token = locator.getUserToken();
final deviceId = locator.getDeviceId();
final appVersion = locator.getAppVersion();
final deviceType = AppUtils().getDeviceType();

options.headers.addAll(
  <String, String>{
    'Authorization': 'Bearer $token',
    'x-device-type': deviceType,
    'x-app-version': appVersion,
    'x-device-id': deviceId
  },
);

if (!isTokenExpire && AppUtils().checkRefreshTokenExpire()) {
  isTokenExpire = true;
  newAccessToken = await _refreshToken();
}

if (newAccessToken != null) {
  options.headers["Authorization"] = "Bearer $newAccessToken";
  newAccessToken = null;
}
handler.next(options);
isTokenExpire = false;

}`

@shawoozy
Copy link

shawoozy commented Sep 5, 2023

same issue here, the refreshing runs more than once...

@chandrabezzo
Copy link

Same issue, I still use mutex to solve this case for refresh token and still use Interceptor instead of QueuedInterceptor.

@cfug cfug locked as off-topic and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
s: feature This issue indicates a feature request
Projects
None yet
Development

No branches or pull requests