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

TF-2769 Fix isolate illegal data #2792

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

tddang-linagora
Copy link
Contributor

@tddang-linagora tddang-linagora commented Apr 10, 2024

Issue

Related documents

Root cause

We are using lib/features/upload/data/network/file_uploader.dart:FileUploader:uploadAttachment:_handleUploadAttachmentAction function to handle upload file in mobile, with the help of Dio, inside a separate isolate with app's main isolate. When this Dio throw DioError exception, by any reason, this exception in turns get sent from this separate isolate to the main isolate. Inside this DioError, the RequestOptions:data is sent back out of this separate isolate, with type Stream, exactly the type we passed into this isolate by File(argsUpload.mobileFileUpload.filePath).openRead(). This type is not supported by send() function of Isolate (see above document), so this isolate throw its own exception, telling us that the type we're letting that isolate call send() is "illegal". Also, we're showing the whole stacktrace of the error if this case happens.

Log

flutter: *** Request ***
flutter: uri: https://jmap.linagora.com/upload/8203ca9793c9c26875c5c1463ad74ec448f7bc416439afce48fc1210e9f1b77a
flutter: method: POST
flutter: responseType: ResponseType.json
flutter: followRedirects: true
flutter: persistentConnection: true
flutter: connectTimeout: null
flutter: sendTimeout: null
flutter: receiveTimeout: null
flutter: receiveDataWhenStatusError: true
flutter: extra: {upload-attachment: {platform: mobile, path: /private/var/mobile/Containers/Data/Application/84C6E2FF-DF6E-42CF-BC42-95435D8C9421/tmp/com.linagora.ios.teammail-Inbox/IMG_1632.PNG, type: image/png, size: 1050986}}

flutter: *** DioError ***:
flutter: uri: https://jmap.linagora.com/upload/8203ca9793c9c26875c5c1463ad74ec448f7bc416439afce48fc1210e9f1b77a
flutter: DioError [bad response]: The request returned an invalid status code of 502.
flutter: uri: https://jmap.linagora.com/upload/8203ca9793c9c26875c5c1463ad74ec448f7bc416439afce48fc1210e9f1b77a
flutter: statusCode: 502
flutter: headers:
flutter:  connection: keep-alive
flutter:  content-type: text/html; charset=utf-8
flutter:  date: Fri, 12 Apr 2024 08:49:28 GMT
flutter:  content-length: 229
flutter:  server: nginx
flutter:  x-apisix-upstream-status: 502

Scenario

The error showing in the demos below is on purpose, to show how the error is handled. In real cases, the attachments will be uploaded as normal.

Before

Screen.Recording.2024-04-10.at.17.54.00.mov

After

Screen.Recording.2024-04-10.at.17.50.22.mov

Copy link

This PR has been deployed to https://linagora.github.io/tmail-flutter/2792.

@chibenwa
Copy link
Member

Cool however can you explain why we get the error in the first place?
Network issue?

@tddang-linagora
Copy link
Contributor Author

Cool however can you explain why we get the error in the first place? Network issue?

I would need to reproduce this bug to understand what went wrong. Will do.

@dab246
Copy link
Member

dab246 commented Apr 11, 2024

IMO, This is not the way to solve this problem. We need to find out why it failed to upload.

@chibenwa
Copy link
Member

IMO, This is not the way to solve this problem. We need to find out why it failed to upload.

WRONG

We need to fix BOTH

@chibenwa
Copy link
Member

(sure I agree with you @dab246 that no error in the first place is non -gegociable however correct error management IS a must.)

@dab246
Copy link
Member

dab246 commented Apr 11, 2024

IMO, This is not the way to solve this problem. We need to find out why it failed to upload.

WRONG

We need to fix BOTH

That's natural. Maybe you don't understand what I mean. WE NEED TO FIND THE CAUSE THAT CAUSED IT TO FAILED. As for the reason for displaying such a long error content in toast message, the above solution has been resolved. BUT EXPECTED IS NOT LIKE THAT.

@chibenwa
Copy link
Member

Sure I 100% that's why I commented in the first place: more investigations on the root cause are also needed.

@tddang-linagora
Copy link
Contributor Author

IMO, This is not the way to solve this problem. We need to find out why it failed to upload.

WRONG

We need to fix BOTH

@chibenwa Added log. This might not be able to resolve from the app side if this is what happened to our tester.

Copy link
Member

@dab246 dab246 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Squash & rebase

@tddang-linagora tddang-linagora force-pushed the fixbug/TF-2769-isolate-illegal-data branch from d163fc7 to 1c3e9ad Compare September 4, 2024 03:38
@tddang-linagora tddang-linagora changed the base branch from v0.11.3-patch4-dev to refactor September 4, 2024 03:38
@dab246
Copy link
Member

dab246 commented Sep 5, 2024

Same root cause with #3129

@hoangdat hoangdat merged commit b4e9ca3 into refactor Sep 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants