Skip to content

Conversation

simolus3
Copy link
Contributor

@simolus3 simolus3 commented Sep 19, 2025

The fetch client turns a ReadableStreamDefaultReader into a Dart Stream by repeatedly invoking reader.read() in an async* function.

This generally works well, but a problem with async* functions is that they can only respect cancellations when yielding. This results in cancellations taking longer than they need to in the following case:

  1. A chunk of data is reported from the reader.
  2. This is emitted by the stream, and we go into the next iteration of the _readBody loop.
  3. Asynchronously, the listener calls .cancel() on its subscription.
  4. This now has to wait for the next chunk of the source stream instead of aborting the request.

This fixes the issue by rewriting the logic to use a StreamController directly.

Closes #1820.

cc @brianquinlan


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:cronet_http 1.5.0 already published at pub.dev
package:cupertino_http 2.4.0-wip WIP (no publish necessary)
package:http 1.6.0-wip WIP (no publish necessary)
package:http2 3.0.0 ready to publish http2-v3.0.0
package:http_multi_server 3.2.2 already published at pub.dev
package:http_parser 4.1.2 already published at pub.dev
package:http_profile 0.1.1-wip WIP (no publish necessary)
package:ok_http 0.1.1-wip WIP (no publish necessary)
package:web_socket 1.0.1 already published at pub.dev
package:web_socket_channel 3.0.3 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
http None 1.5.0 1.6.0-wip 1.5.0 ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Coverage ⚠️
File Coverage
pkgs/http/lib/src/browser_client.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/http/example/main.dart

This check can be disabled by tagging the PR with skip-license-check.

Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

Very high quality PR! Especially the added conformance test (a few of which seem to be failing).

@simolus3
Copy link
Contributor Author

It looks like the remaining failures are all infra or appear to be unrelated to my changes. Let me know if there's anything I should look into.

@brianquinlan
Copy link
Collaborator

It looks like the remaining failures are all infra or appear to be unrelated to my changes. Let me know if there's anything I should look into.

Yep, these have nothing to do with your change.

Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

Hey @simolus3 ,

I just wanted to let you know what I'm still looking at your change. I've included a few nit picky comments but overall its excellent. What I don't like is the increased complexity of this client. But I can't think of a better way of doing this so probably it is fine the way it is. But it might take me a few days to convince myself of that ;-)

@simolus3 simolus3 force-pushed the fetch-response-stream-cancellation branch from d16b9a0 to 2199424 Compare September 23, 2025 20:33
Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

I think it's pretty good. I can simplify it a little, but the underlying idea is the same.

@simolus3
Copy link
Contributor Author

Alright, I have adopted most of the suggestions shared by @lrhn. I think they make the control flow a lot more obvious 👍

I think the correct behavior (which we also had with the old async* implementation) is to only call getReader() when the stream is actually listened to, so I've moved most of the setup into the onListen callback. That also avoids the use of finalizers here.

Despite being a single-use stream, I think using Stream.multi may be helpful here to be explicit about adding events synchronously? I've also added a comment on each addSync / addErrorSync / closeSync invocation to explain why it's safe.

@brianquinlan
Copy link
Collaborator

It looks like one test is failing on WASM.

@simolus3
Copy link
Contributor Author

simolus3 commented Oct 7, 2025

Thanks, I've fixed that test.

@brianquinlan
Copy link
Collaborator

It looks like packages cupertino_http and cronet_http both fail on "cancel paused stream via abortable request". Let me see if I can figure out why...

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fetch client: Cancelling response subscriptions waits for next chunk

3 participants