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

Bug with the clone() method while retrying FormData requests #1902

Closed
anilslabs opened this issue Jul 24, 2023 · 13 comments
Closed

Bug with the clone() method while retrying FormData requests #1902

anilslabs opened this issue Jul 24, 2023 · 13 comments
Labels
fixed p: dio Targeting `dio` package s: bug Something isn't working

Comments

@anilslabs
Copy link

Package

dio

Version

5.3.0

Output of flutter doctor -v

Sookshums-MacBook-Pro:Digital-Health-Passport sookshumlabs$ flutter doctor -v
[✓] Flutter (Channel stable, 3.10.5, on macOS 12.6 21G115 darwin-x64, locale en-IN)
    • Flutter version 3.10.5 on channel stable at /Users/sookshumlabs/development/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 796c8ef792 (6 weeks ago), 2023-06-13 15:51:02 -0700
    • Engine revision 45f6e00911
    • Dart version 3.0.5
    • DevTools version 2.23.1

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.1)
    • Android SDK at /Users/sookshumlabs/Library/Android/sdk
    • Platform android-33, build-tools 33.0.1
    • ANDROID_HOME = /Users/sookshumlabs/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.8+10-b944.6916264)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.0)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14A309
    • CocoaPods version 1.11.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.8+10-b944.6916264)

[✓] VS Code (version 1.80.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension can be installed from:
      🔨 https://marketplace.visualstudio.com/items?itemName=Dart-Code.flutter

[✓] Connected device (4 available)
    • CPH2467 (mobile)   • ab2ba7e2                             • android-arm64  • Android 13 (API 33)
    • iPhone 14 (mobile) • 1B407168-676E-4547-AE54-14AA0C4D55D3 • ios            • com.apple.CoreSimulator.SimRuntime.iOS-16-0 (simulator)
    • macOS (desktop)    • macos                                • darwin-x64     • macOS 12.6 21G115 darwin-x64
    • Chrome (web)       • chrome                               • web-javascript • Google Chrome 114.0.5735.198

[✓] Network resources
    • All expected network resources are available.

• No issues found!

Dart Version

3.0.5

Steps to Reproduce

  1. Create a POST request to upload a File with an expired token.
  2. Refresh the token using a custom interceptor and retry the failing API request.
onError: (error, handler) async {
  String _refreshTokenPath = instanceUrl() + refreshToken();

  if (error.requestOptions.path != _refreshTokenPath || error.response?.statusCode != 401) {
    return handler.next(error);
  }

  var token = await updateRefreshToken();
  if (token.isEmpty) {
    return handler.next(error);
  }

  if ([nullLocalTokenString, nullServerTokenString, nullServerSessionTokenString].contains(token)) {
    // 401 status;
    // Local token missing
    // New session token not generated
    // Log user out and push login screen
    var _navigationService = serviceLocator<NavigationService>();
    if (_navigationService.navigationKey.currentContext != null) {
      var context = _navigationService.navigationKey.currentContext;
      refreshTokenLoginUserAlertDialog(context!);
    }
  } else {
    error.requestOptions.headers['Authorization'] = 'Bearer $token';
    final opts = Options(
      method: error.requestOptions.method,
      headers: error.requestOptions.headers,
    );

    try {
      if (error.requestOptions.data is FormData && file != null) {
        FormData formData = FormData.fromMap(Map.from(error.requestOptions.data));
        formData.files.add(MapEntry('file', file.clone()));
        error.requestOptions.data = formData;
      }

      final retryFailingRequest = await dio.request<dynamic>(
        error.requestOptions.path,
        options: opts,
        data: error.requestOptions.data,
      );
      return handler.resolve(retryFailingRequest);
    } catch (_error) {
      logger.i(_error);
      return handler.next(error);
    }
  }
}

Expected Result

The file should be uploaded to the server with an updated user session token.

Actual Result

Getting the following exception:

DioException [unknown]: null
Error: FileSystemException: File closed, path = '/Users/sookshumlabs/Library/Developer/CoreSimulator/Devices/1B407168-676E-4547-AE54-14AA0C4D55D3/data/Containers/Data/Application/EBB5CAD9-DF62-4F2E-B57D-98FD2083417C/Documents/test_careplan_file.pdf'
@anilslabs anilslabs added h: need triage This issue needs to be categorized s: bug Something isn't working labels Jul 24, 2023
@kuhnroyal
Copy link
Member

Related to #1889
@gabrielaraujoz you mind taking a look?

@gabrielaraujoz
Copy link
Contributor

@kuhnroyal @anilslabs I'm taking a look at it but I have a feeling already of what could be going on.

@anilslabs
Copy link
Author

@gabrielaraujoz Basically, I'm trying to clone the multipart file in the error handler and retrying the unsuccessful request with an updated session token.

@gabrielaraujoz
Copy link
Contributor

@anilslabs thanks buddy, I could reproduce it. @kuhnroyal I believe the issue lies in us reusing the stream when cloning, specially when using the fromFile() and fromFileSync methods, because in multipartFileFromPath and multipartFileFromPathSync we create a stream from File.openRead() and it is used when making the first request. I tried the following solution and it seems to be working:

  1. Store file path and the List<int> bytes value in the class. Note: I could not find a way to do that other than adding two more optional fields to the base constructor, maybe you could help me with that? The code looks like this:
MultipartFile(
    Stream<List<int>> stream,
    this.length, {
    this.filename,
    MediaType? contentType,
    Map<String, List<String>>? headers,
    String? filePath,
    List<int>? value,
  })  : _stream = stream,
        headers = caseInsensitiveKeyMap(headers),
        contentType = contentType ?? MediaType('application', 'octet-stream'),
        _filePath = filePath,
        _valueBytes = value;
  1. In the clone method, create a new instance using the same ways we initially create the MultipartFile, like this:
MultipartFile clone() {
    if (_filePath != null) {
      return multipartFileFromPathSync(
        _filePath!,
        filename: filename,
        contentType: contentType,
        headers: headers,
      );
    } else if (_valueBytes != null) {
      return MultipartFile.fromBytes(
        _valueBytes!,
        filename: filename,
        contentType: contentType,
        headers: headers,
      );
    } else {
      throw ArgumentError(
          'Operation is not possible, file path or value is not available');
    }
  }

WDYT? I'll open a PR with this code for you to check.

@kuhnroyal
Copy link
Member

What if we deprecate the stream parameter and add a new Stream<List<int>> Function() data provider parameter. This way we can just build a new stream?

@gabrielaraujoz
Copy link
Contributor

@kuhnroyal I'm not sure it resolve this issue when it comes to the File.openRead since it already returns a stream, but we can certainly try.

@kuhnroyal
Copy link
Member

Well if it is a Stream provider function, then you get a new Stream every time you call it.

@gabrielaraujoz
Copy link
Contributor

I'll look into it @kuhnroyal - in the meanwhile, I've tested the proposed solution from the PR in my production app in stg and it worked as intended!

@gabrielaraujoz
Copy link
Contributor

@kuhnroyal you're amazing, the solution with the stream provider function worked! I already updated the PR and also:

  • Added convenience method for cloning FormData as a whole
  • Added regression test with the new method to guarantee we don't get that exception anymore when cloning FormData

@gabrielaraujoz
Copy link
Contributor

@anilslabs hey, this PR should fix your issue in case you're in an emergency: #1903

@anilslabs
Copy link
Author

@gabrielaraujoz Thanks. I have tested with the PR and the issue is fixed. Awaiting a pub.dev release.

@gabrielaraujoz
Copy link
Contributor

@AlexV525 hey Alex, sorry to bother you, but do we have any forecast on when we'll be able to have the fix released?

@AlexV525
Copy link
Member

Generally I'll do a week cycle for publishing. You can use overrides to use the patch if it's urgent.

@AlexV525 AlexV525 added p: dio Targeting `dio` package fixed and removed h: need triage This issue needs to be categorized labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed p: dio Targeting `dio` package s: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants