QueuedInterceptor is flawed, which causes dead lock by design.

[timnew]

New Issue Checklist

• I have searched for a similar issue in the project and found none

Issue Info

Info	Value
Dio Version	dio 4.0.6.

Issue Description and Steps

I uses QueuedInterceptor to handle access token, very standard scenario:

• Add Authorization header in onRequest
• Refresh accessToken if server responds 401 in onError
• Give up if server kept responding 401 after retried.

Code is very similar to official example: https://github.com/flutterchina/dio/blob/develop/example/lib/queued_interceptor_crsftoken.dart

As the code performs an retry as following code shown:

  @override
  Future<void> onError(
    DioError err,
    ErrorInterceptorHandler handler,
  ) async {
    // ... check whether should refresh token, if not return
    // ... refresh token
    // ... check whether should retry, if not return

    _logger.info('Retry request');
    final retryRequestOptions = error.requestOptions._createRetryRequestionOptions();
    await dio.fetch(retryRequestOptions).then(
          handler.resolve,
          onError: (error) => handler.reject(error as DioError),
        );
  }

Unfortunately, this code won't work, and it naturally causes dead lock by design:

• It calls fetch on the same dio instance before onError is finished.
• Due to how QueuedInterceptor works, calls to onError will be queued, next one won't be called until current one finished.
• If a new error is raised during "retry", it would trigger an new onError requests, but it won't be processed because current onError is still running.
• And the same time, current onError won't finish, as it depends dio.fetch to return, which requires onError returns first.
• Dead lock!

I might be wrong, but it seems there is no solution to this bug, and InterceptorLock is marked as deprecated, so no idea how this would be done properly.

[timnew]

Built a hacky workaround by separating the retry logic to a following normal interceptor.
The idea is

1. rather than to retry in QueuedInterceptor, which caused deadlock. Just do the logic necessary (such as refresh token), and prepare the RequestOptions for retry, and pass it down to next Interceptor.
2. As the QueuedInterceptor doesn't do retry, so the lock is resolved.
3. A normal RetryInterceptor following check the retry signal, and do the retry when necessary

So after the change, the onError become

  @override
  Future<void> onError(
    DioError err,
    ErrorInterceptorHandler handler,
  ) async {
    // ... code unrelated ...
    final requestOptions = error.requestOptions;

    _logger.info('Schedule retry');
    requestOptions._markNeedRetry(retryRequestOptions);
    handler.next(err);

    // Code causes deadlock

    //  _logger.info('Retry request');
    // dio.fetch(retryRequestOption).then(
    //       handler.resolve,
    //       onError: (error) => handler.reject(error as DioError),
    //     );
  }

So the helper methods and RetryInterceptor:

  
const _retryRequestOptionKey = 'retryRequestOption';

extension RequestOptionsExtension on RequestOptions {
  void _markNeedRetry(RequestOptions retryRequestOption) {
    extra[_retryRequestOptionKey] = retryRequestOption;
  }

  RequestOptions? _checkNeedRetry() {
    final result = extra[_retryRequestOptionKey] as RequestOptions?;
    extra.remove(_retryRequestOptionKey);
    return result;
  }
}

class AuthRetryInterceptor extends Interceptor {
  final Dio dio;

  AuthRetryInterceptor(this.dio);

  @override
  void onError(DioError err, ErrorInterceptorHandler handler) {
    final retryRequestOptions = err.requestOptions._checkNeedRetry();

    if (retryRequestOptions == null) {
      return handler.next(err);
    }

    _logger.info('Retry request');
    dio.fetch(retryRequestOptions).then(
          handler.resolve,
          onError: (error) => handler.reject(error as DioError),
        );
  }
}

And this is dio setup:

dio.addInterceptors([
  authInterceptor,
  AuthRetryInterceptor(dio),
]);

[kostadin-damyanov-prime]

Usually this is solved by using another (plain) instance of the Dio client which does not have the same interceptor to do the token refreshing or whatever calls you need to do before you exit the QueuedInterceptor.

[AlexV525]

Yes. A proper solution was like @kostadin-damyanov-prime mentioned using another Dio instance only for token updates.

[timnew]

Hey @kostadin-damyanov-prime @AlexV525, it seems you get the issue wrong:

The dead lock DOESN'T happen on the token refreshing but on RETRYING.
So using another Dio instance to refresh token WON'T prevent the deadlock.
Because the deadlock happens on ANY kind of error on RETRYING, could be caused by server returns 500, or it just returns another 401, or could be as simple as network timeout, or even any interceptor raised an error.

Once the error happens, whole network stack would be deadlocked, until you recreate the whole dio instance. If dio is hold as singleton by the app, it pretty much means user have to kill the whole app, or any requests sent by this dio will be blocked.

As the deadlock happens in retry, where you would like to have the same interceptor chain. So using another dio seems odd, unless you create a new dio instance every time when retry happens. Seems not a good practise though.

[kostadin-damyanov-prime]

@timnew I see. I think the issue is caused by the following:

• QueuedInterceptor is blocking by design. This means that if a request is executing code in the interceptor then the other requests are blocked and waiting for the current request to exit the interceptor.
• You are using await in the retry error case. This adds a new request to the dio instance, which is currently blocked because it is waiting for the await call to return.

If you take a look at this code posted by the PRs author you will see they use then instead of await to retry the current call after the refreshing is done. This way the current call will set up a callback and exit the interceptor so the retry call can be executed next.

Note: I think the linked code example has another isue, which is tha tit ONLY uses then and no await at all. This is problematic in the case where there are multiple requests at once and the token needs to be refreshed. In this case the queue will not be blocked at all and the token will be refreshed the same number of times as the number of requests we have. I believe this will be solved by using the following in onError():

• Use await to refresh the token. This will block the request queue and make the other requests wait for the refreshing to be done. This requires you to use another dio instance because the current one will be blocked.
• After the refreshing is done just use then to retry the request that caused the 401 error. You may reuse the same dio instance because the queue will not be blocked.

[timnew]

@kostadin-damyanov-prime Thanks for the detailed explanation, I see what you mean.
For the code sample it might be better to written in a way as following:

 @override
  Future<void> onError(
    DioError err,
    ErrorInterceptorHandler handler,
  ) async {
    // ... code unrelated ...
    final requestOptions = error.requestOptions;

    // do things requires look
    final newToken = await _refreshToken(); // Await to block this method.
    ]
     
    // retry in a unblock way
    _retryRequest(requestOptions._markAsRetry(), handler).ignore(); // Retry in a new microtask and mark it as ignored.
  }

  Future<void> _retryRequest(
    RequestOptions retryRequestOption,
    ErrorInterceptorHandler handler,
  ) async {
    _logger.info('Retry request');
    await dio.fetch(retryRequestOption).then(
      handler.resolve,
      onError: (error) => handler.reject(error as DioError),
    );
  }

[timnew]

To be honest, I feel QueuedInterceptor is a overkill:

1. It doesn't prevent human mistake, instead it actually leads to human mistake, as people would tends to await the async functions
2. With this level of complexity, I just need to simply wrap the _refreshToken with a lazy completer, then I would have a parallel lock. It is much easier to be used, and less error prone, and we don't need to the complexity of the QueuedInterceptor introduced.
3. Dio instance have to differentiate QueuedInterceptor from normal interceptors, as it has to call some different private method to ensure the task is created in queue, rather than get executed immediately, which seems not to be a good design.

[timnew]

Something like this would be enough to prevent the parallel call.

Completer<String>? _refreshTokenCompleter;
Future<String> refreshToken() {
  if(_refreshTokenCompleter != null) return _refreshTokenCompleter!.future;
  
  final completer = _refreshTokenCompleter = Completer();

  try {
    final token = await _refreshToken();
    completer.complete(token);
    return token;
  } catch (ex, stacktrace) {
    completer.completeError(ex, stacktrace);
    rethrow;
  }
}
Future<String> _refreshToken() {
  // do actual refresh token logic here
}

[hemantchauhan]

Hello @kostadin-damyanov-prime, you mentioned

Note: I think the linked code example has another issue, which is that it ONLY uses then and no await at all. This is problematic in the case where there are multiple requests at once and the token needs to be refreshed.

I am not sure steps mentioned in the note will still work in case of multiple requests. Once say 3 concurrent requests get queued in the QueuedInterceptor on token expiry, & first one using await for refreshToken & .then for retry.
As handler.resolve or handler.next is called on success of retry, interceptor will queue in the 2nd requests onError and same process will repeat again? How will the 2nd request know in OnError to go for retry instead of refreshToken?

class RefreshTokenInterceptor extends QueuedInterceptor {
  final Dio dio;
  AccessTokenStorage tokenStorage = AccessTokenStorage();
  RefreshTokenInterceptor(this.dio);

  @override
  Future<void> onError(DioError err, ErrorInterceptorHandler handler) async {
    if (err.response?.statusCode == 401) {
      await refreshToken(err, handler);
    } else {
      handler.next(err);
    }
  }

  Future refreshToken(DioError err, ErrorInterceptorHandler handler) async {
    String? refreshToken = await tokenStorage.getRefreshToken();

    if (refreshToken == null) {
      handler.reject(err);
      return;
    }

 var tokenDio = Dio();
// setup headers, base urls & request for tokenDio
    try {
      Response response =
          await tokenDio.post('/${path}', data:requestBody);

      if (response.statusCode == 201) {
        AccessTokenStorage.setRefreshToken(response.data['refresh_token']);
        AccessTokenStorage.setAccessToken(response.data['access_token']);

        await retryCurrent(tokenDio, err.requestOptions, handler);
      } else {
        handler.reject(err);
      }
    } catch (error) {
      handler.reject(err);
    }
  }

  Future<void> retryCurrent(Dio tokenDio, RequestOptions requestOptions,
      ErrorInterceptorHandler handler) async {
    await tokenDio.fetch(requestOptions).then(
          handler.resolve,
          onError: (error) => handler.reject(error as DioError),
        );
  }
}

I just started flutter a month ago and spent last week looking for the right solution for this. let me know if what I have written is ok or not.
@timnew

[bishal-rumba007]

I am still facing the same issue while making retry. any update on how its done??? @timnew

[timnew]

I just simply give up the QueuedInterceptor, it doesn't bring much value in but cause lots of trouble.
I can achieve similar lock mechanism inside of a normal interceptor with a Completer

[xleon]

@timnew workaround with the completer made the trick for me.
I spent a long time figuring out issues with QueuedInterceptor

[AristideVB]

@timnew would you mind sharing you're full refresh implementation ? Why do you think QueuedInterceptor has drawbacks ?