-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(mobile): upload file operation hang #17990
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
Conversation
|
Why are you adding these timeouts? Downloading an iCloud asset can take a while and this is normal. |
|
There is a bug on iOS where stale iCloud Shared Albums' assets get downloaded but then hang. The synchronous operation of reading a corrupted file stream results in a hang during the upload process. There was no catch to detect it We can probably make the iCloud download timeout a user-defined option |
|
Just make it so it times out if there's no progress for a certain amount of time. We shouldn't assume everyone has very fast internet connections. |
|
This is a temporary workaround, when we have the proper data sync and use the background_downloader, parallel processing will be used and each request will have its own timeout |
|
I'm saying this isn't a viable solution, even temporarily. If someone is downloading a video from iCloud, this PR will give them only 30s to do so or things will go south. It's arguably more problematic than what it aims to fix because it will affect more people. |
|
No, iCloud download has 5 minutes timeout, 30 seconds is for reading the file stream in the request |
|
That's still just wrong. If they're downloading a 2GiB file, they need at least 6.82MiB/s download speed or it will get stuck and waste time and bandwidth. Any timeout based on total download duration is an assumption of internet speed that we shouldn't impose. |
| fileStream, | ||
| file.lengthSync(), | ||
| filename: originalFileName, | ||
| final assetRawUploadData = await Future.sync(() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, have you tested this? A timeout enforced by the event loop shouldn't work if the code blocks the event loop.
|
Based on Merts comments I think we agreed this isn't the way to go, we should have some kind of check to see if data transfer is still ongoing rather than arbitrary time limits. Closing for now |
Add timeouts to synchronous operations that can potentially hang during upload.