GeezerVison is showing github.com — Add `timeout` parameter for downloading by an920107 · Pull Request #482 · Baseflow/flutter_cache_manager
Conversation
an920107
commented
Feb 19, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
This PR is to resolve #141, supporting pass timeout for downloading.
⤵️ What is the current behavior?
There is no way to set timeout.
🆕 What is the new behavior (if this is a feature change)?
A timeout duration can be set, and the http request will be timeouted after the duration (a TimeoutException will be thrown). If timeout is not set, all behaviors are same with the original.
💥 Does this PR introduce a breaking change?
N/A for flutter_cache_manager (see follow-up note below for flutter_cache_manager_firebase).
🐛 Recommendations for testing
- Check if it will stop and throw a
TimeoutException after the duration.
- Check if the behavior is same as original when
timeout is not set.
📝 Links to relevant issues/docs
Issue #141.
🤔 Checklist before submitting
- All projects build
- Follows style guide lines (code style guide)
- Relevant documentation was updated
- Rebased onto current develop
- Version bumped and dated CHANGELOG entry added
⚠️ Follow-up needed in flutter_cache_manager_firebase (separate package)
This change adds Duration? timeout to FileService.get. flutter_cache_manager_firebase overrides that method in FirebaseHttpFileService without timeout, so once this is released as 3.5.0 (its constraint flutter_cache_manager: ^3.4.2 allows 3.5.0) that override becomes an invalid_override compile error. A follow-up PR will add timeout there and forward headers/timeout to super.get. Please hold the v3.5.0 tag until that follow-up lands. This PR only touches flutter_cache_manager.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Gustl22
commented
Aug 6, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
@martijn00 can we have this reviewed? Thans for the time effort :D
Or is this already possible through intercepting dio / http service?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
main has moved since you opened this. #518 (758e206), #520 (313bfa1), f803860 and the formatting pass in cce9a65 changed lib/src/cache_manager.dart, lib/src/cache_managers/image_cache_manager.dart and lib/src/web/web_helper.dart, which your pull request also changes, so you are better placed than we are to resolve it. Could you merge main into your branch? The other conflicts are formatting and the generated test/mock.mocks.dart, which dart format . and dart run build_runner build --delete-conflicting-outputs take care of. Happy to help if anything is unclear.
|
an920107 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks @rickdijk — merged main into feature/timeout and pushed. Resolved the conflicts by keeping the timeout parameter while adopting the new formatting, regenerated test/mock.mocks.dart with build_runner, and added the missing 3.5.0 version bump + dated CHANGELOG entry (per #532). I also fixed getImageFile so timeout is actually forwarded on the resized-image path (it was silently dropped there).
One thing to flag before release: flutter_cache_manager_firebase's FirebaseHttpFileService.get overrides FileService.get without the new timeout, so publishing 3.5.0 will turn that override into an invalid_override compile error. I'll open a follow-up PR there (add timeout and forward headers/timeout to super.get); please hold the v3.5.0 tag until it lands.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Follow-up draft PR for flutter_cache_manager_firebase: #538 — adds timeout to FirebaseHttpFileService.get, forwards headers/timeout to super.get, and raises the dependency constraint to ^3.5.0.
It depends on this PR (flutter_cache_manager 3.5.0), so its CI stays red until 3.5.0 is published.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Few comments on this PR:
-
Queued requests run with the wrong timeout. When more than 10 downloads are running, a new request waits in a queue. The queue doesn't store its timeout, so it later runs with the timeout of whichever download just finished.
-
This is a breaking change shipped as a minor version. Adding timeout to the abstract FileService.get breaks every class that overrides it. That includes this repo's own FirebaseHttpFileService, which accepts ^3.4.2 and so would pick up 3.5.0 and fail to compile.
-
The timeout only covers the wait for response headers, so a stalled body download still hangs.
-
A timed-out request is never cancelled, so its connection stays open.
-
A caller that joins a download already in progress has its timeout ignored.
-
getSingleFile doesn't accept a timeout at all.
-
There are no tests for the new behavior
I'm pretty convinced it would be simpler fix to set a timeout once on HttpFileService instead of passed through five layers. That won't break anything and avoids the problems mentioned above.
Let me know what your thoughts are :)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Requested changes must be addressed to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
a way to setup the request timeout
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
rickdijk
left a comment