Skip to content

[flutter_svg] Remove failed async loads from Cache._pending - #13604

Merged
auto-submit[bot] merged 1 commit into
flutter:mainfrom
apwilson:fix-flutter-svg-cache-pending
Nov 11, 2026
Merged

auto-submit[bot] merged 1 commit into
flutter:mainfrom
apwilson:fix-flutter-svg-cache-pending

Conversation

@apwilson

@apwilson apwilson commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

In Cache.putIfAbsent, pendingResult.then((ByteData data) { ... }) did not attach an onError handler. When an asynchronous SVG load rejected (for example, SvgNetworkLoader failing while offline or throwing http.ClientException on non-2xx responses), _pending.remove(key) never ran—leaving the rejected Future permanently cached in _pending[key] across retries—and the unguarded child Future from .then reported an uncaught error to the active Zone even when callers awaited putIfAbsent inside try/catch.

Attaches onError to pendingResult.then so failed loads evict _pending[key] without leaking uncaught Zone errors while preserving the caller's rejected Future.

Fixes b/583860731

Pre-Review Checklist

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2

@github-actions github-actions Bot added p: flutter_svg triage-engine Should be looked at in engine triage labels Nov 9, 2026

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates the 'flutter_svg' package to version 2.3.3. It modifies the 'Cache.putIfAbsent' method in 'cache.dart' to handle asynchronous load failures by adding an 'onError' callback that removes the failed future's key from the '_pending' map, preventing rejected futures from remaining in the cache and leaking uncaught zone errors. A corresponding unit test has been added to 'cache_test.dart' to verify this behavior, and the 'CHANGELOG.md' and 'pubspec.yaml' files have been updated accordingly. There are no review comments, so no additional feedback is provided.

@gaaclarke
gaaclarke self-requested a review November 10, 2026 17:39

@jtmcdole jtmcdole left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm-seinfeld

@jtmcdole jtmcdole added the CICD Run CI/CD label Nov 10, 2026
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
@auto-submit

auto-submit Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/13604, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@jtmcdole jtmcdole added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
@auto-submit

auto-submit Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/13604, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm! thanks

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 11, 2026
@auto-submit
auto-submit Bot merged commit 99943dd into flutter:main Nov 11, 2026
13 checks passed
opensource-fork-sync Bot pushed a commit to cla7aye15I4nd/opensource-flutter__flutter that referenced this pull request Nov 11, 2026
…er#196381)

flutter/packages@08faf64...653628e

2026-11-11 [email protected] Roll Flutter from
fffd8c2 to b98afb5 (31 revisions) (flutter/packages#13623)
2026-11-11 [email protected] [material_ui] Document how to
programmatically clear DropdownMenu selection (flutter/packages#13266)
2026-11-11 [email protected] [pigeon] Fix basePath resolution when
paths share a string prefix with basePath (flutter/packages#13342)
2026-11-11 [email protected]
[cupertino_ui] Fix CupertinoSheetRoute throwing when a drag ends during
a pending relayout (flutter/packages#13316)
2026-11-11 [email protected] [shared_preferences] Updates Pigeon to
30.0.1 to support multi-engine registration and deregistration.
(flutter/packages#13617)
2026-11-11 [email protected]
[vector_graphics] Catch any throwable in useHtmlRenderObject() and cache
HTML fallback (flutter/packages#13545)
2026-11-11 [email protected] [cupertino_ui]
Include selectionHandleColor in NoDefaultCupertinoThemeData equality
(flutter/packages#13450)
2026-11-11 [email protected] [flutter_svg] Remove failed async loads
from Cache._pending (flutter/packages#13604)
2026-11-11 [email protected] [pigeon] Adds Native Interop FFI codec
serialization for NS types (flutter/packages#13616)
2026-11-10 [email protected] [pigeon] Support multi-engine Native
Interop registration and instance-aware unregistration
(flutter/packages#13576)
2026-11-10 [email protected] [image_picker_ios] Convert the
iOS example app and UI tests to Swift (flutter/packages#13570)
2026-11-10 [email protected]
[cupertino_ui] Localize CupertinoActionSheet semantics label
(flutter/packages#13377)
2026-11-10 [email protected] [camera_android]
Fix stopVideoRecording returning JPEG when takePicture runs during
recording (flutter/packages#13070)
2026-11-10 [email protected] [google_sign_in_android] Rename keep.xml
to google_sign_in_keep.xml (flutter/packages#13475)
2026-11-10 [email protected] [web_benchmarks] Fix benchmark stalling
when only repainting views that are dirty (flutter/packages#13089)
2026-11-10 [email protected] Roll Flutter from
d463821 to fffd8c2 (21 revisions) (flutter/packages#13614)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit Bot pushed a commit that referenced this pull request Nov 11, 2026
…3595)

Rebased on current main (`8041ccab1`), preserving the failed-load fix and its regression test from #13604. This PR adds invalidation of pending loads on `Cache.clear` and `Cache.evict`: an obsolete completion cannot repopulate the cache or remove a replacement request, even when loaders reuse the same Future.

Documents pending-load invalidation on both methods. The cache lifecycle tests live in `cache_test.dart`. The shared-Future regression checks that an evicted request does not populate the cache before the replacement's completion callback runs. Preserves synchronous cache hits, coalescing and entry-count LRU. Bumps the package to 2.3.4 with a changelog entry.

Related: flutter/flutter#182486 (failed-load cleanup is already on main via #13604).

Validation on Windows: `flutter analyze` clean; full `flutter test` suite: 77 passed, 1 existing skip; formatting and `git diff --check` passed. No golden files changed.

Overlapping proposals: #13613 also concerns failed pending loads; main already includes #13604. This PR retains that fix and focuses on clear/evict invalidation. #13596 is stacked on this branch and was rebased with it.

## Pre-Review Checklist

This draft was prepared with AI assistance and remains pending the contributor's human review and confirmation of the unchecked checklist items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD p: flutter_svg triage-engine Should be looked at in engine triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants