Repository navigation
Cancel codec subscription when image completer is disposed - #1064
somiljain2006 wants to merge 1 commit into
Conversation
|
|
||
| __disposed = true; | ||
|
|
||
| _codecSubscription?.cancel(); |
There was a problem hiding this comment.
This cancel can leak a codec that is still being decoded. _load is an async* generator. If the last listener drops while it is suspended in await decode(bytes), the cancel takes effect there, and the following yield decoded returns without delivering anything. The decoded ui.Codec is dropped and nobody disposes it. The new event.dispose() guard in onData is meant for exactly that codec, but with the subscription cancelled it never runs.
This also changes behavior. The cancel propagates into the cache manager's getFileStream/getImageFile subscription inside _load. An image that loses its last listener mid-download (fast scrolling) no longer finishes fetching and decoding, so scrolling back may refetch.
| @@ -240,7 +247,8 @@ class MultiImageStreamCompleter extends ImageStreamCompleter { | |||
| } | |||
|
|
|||
| __disposed = true; | |||
There was a problem hiding this comment.
While disposing: __maybeDispose still leaves the resources the completer already holds alive: _codec, _nextImageCodec and _nextFrame. They should probably be released here (_codec?.dispose(), _nextImageCodec?.dispose(), _nextFrame?.image.dispose()). That would also make the onData guard's event.dispose() consistent with the rest of disposal.
| codec.listen( | ||
| _codecSubscription = codec.listen( | ||
| (event) { | ||
| if (__disposed) { |
There was a problem hiding this comment.
These two if (__disposed) guards (here and in onError) can't be reached. __disposed is set in the same synchronous block that cancels the subscription, and a cancelled subscription delivers no further events, including ones already queued. Either keep the subscription and rely on the guards, or cancel and drop the guards, but not both. No test exercises these branches either.
| }) : _informationCollector = informationCollector { | ||
| this.debugLabel = debugLabel; | ||
| codec.listen( | ||
| _codecSubscription = codec.listen( |
There was a problem hiding this comment.
Does this address #1023? That issue is a 404 on Windows (Failed to post message to main thread, plus a crash) while the widget is on screen and has listeners. In that case __disposed is false, and the error goes through onError exactly as before. The "confirm errorWidget is displayed" testing step doesn't exercise anything this diff changes.
| ); | ||
| }); | ||
|
|
||
| test('Completer unsubscribes from codec stream when disposed', () async { |
There was a problem hiding this comment.
These tests only check hasListener. Deleting both if (__disposed) guards keeps them green, and so does the codec leak in the generator. Could you add a test with an async* source that decodes, gets cancelled mid-decode, and then asserts the emitted codec is disposed? Plus one asserting that no error is reported after disposal?
| completer.addListener(listener); | ||
| completer.removeListener(listener); | ||
|
|
||
| // Allow the stream subscription cancellation to finish. |
There was a problem hiding this comment.
Nit: StreamController.hasListener turns false synchronously when cancel() is called, so this delay isn't needed, and the comment suggests cancellation is asynchronous when it isn't.
|
This changes |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix: Properly manage the codec stream subscription lifecycle in
MultiImageStreamCompleterto prevent callbacks from being processed after the completer has been disposed.⬇️ What is the current behavior?
The codec stream subscription is not explicitly retained or cancelled when the image completer is disposed. This can allow asynchronous codec events to arrive after disposal, potentially causing lifecycle issues and contributing to errors during image loading.
🆕 What is the new behavior? (If this is a feature change)
N/A
💥 Does this PR introduce a breaking change?
No. This is an internal lifecycle-management fix and does not change the public API.
🐛 Recommendations for testing
errorWidgetis displayed as expected.📝 Links to relevant issues/docs
🤔 Checklist before submitting