Skip to content

Add timeout parameter for downloading - #482

Open
an920107 wants to merge 7 commits into
Baseflow:mainfrom
an920107:feature/timeout
Open

Add timeout parameter for downloading#482
an920107 wants to merge 7 commits into
Baseflow:mainfrom
an920107:feature/timeout

Conversation

@an920107

@an920107 an920107 commented Feb 19, 2025 •
edited
Loading

Copy link
Copy Markdown

✨ 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

  1. Check if it will stop and throw a TimeoutException after the duration.
  2. 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.

@an920107
an920107 marked this pull request as ready for review February 19, 2025 04:04
@an920107 an920107 mentioned this pull request Feb 19, 2025
Open
2 tasks done
@Gustl22

Gustl22 commented Aug 6, 2025 •
edited
Loading

Copy link
Copy Markdown

@martijn00 can we have this reviewed? Thans for the time effort :D
Or is this already possible through intercepting dio / http service?

@xmattjus xmattjus mentioned this pull request Mar 2, 2026
Closed
@rickdijk
rickdijk changed the base branch from develop to main September 16, 2026 00:23
@rickdijk

rickdijk commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

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 an920107 closed this Oct 2, 2026
@an920107 an920107 reopened this Oct 2, 2026
@an920107

an920107 commented Oct 2, 2026

Copy link
Copy Markdown
Author

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.

@an920107 an920107 mentioned this pull request Oct 2, 2026
Draft
3 of 5 tasks
@an920107

an920107 commented Oct 2, 2026

Copy link
Copy Markdown
Author

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.

rickdijk
rickdijk requested changes Oct 3, 2026
View reviewed changes

@rickdijk rickdijk left a comment

Copy link
Copy Markdown
Contributor

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rickdijk rickdijk rickdijk requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

a way to setup the request timeout

3 participants

@an920107 @Gustl22 @rickdijk