feat(cupertino_http): shared session#1876
feat(cupertino_http): shared session#1876mertalev wants to merge 16 commits intodart-lang:masterfrom
Conversation
github-actions
bot
added
package:cupertino_http
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with 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.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with
Unused Dependencies
|
| Package | Status |
|---|---|
| cupertino_http | ❗ Show IssuesThese packages are used outside lib/ but are not dev_dependencies: |
For details on how to fix these, see dependency_validator.
This check can be disabled by tagging the PR with skip-unused-dependencies-check.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/cupertino_http/example/integration_test/client_conformance_test.dart | 💔 Not covered |
| pkgs/cupertino_http/example/integration_test/client_test.dart | 💔 Not covered |
| pkgs/cupertino_http/lib/src/cupertino_api.dart | 💔 Not covered |
| pkgs/cupertino_http/lib/src/cupertino_client.dart | 💔 Not covered |
| pkgs/cupertino_http/lib/src/cupertino_web_socket.dart | 💔 Not covered |
| pkgs/cupertino_http/lib/src/native_cupertino_bindings.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.
Changelog Entry ❗
| Package | Changed Files |
|---|---|
| package:cupertino_http | pkgs/cupertino_http/lib/src/cupertino_api.dart pkgs/cupertino_http/lib/src/cupertino_client.dart pkgs/cupertino_http/lib/src/cupertino_web_socket.dart pkgs/cupertino_http/lib/src/native_cupertino_bindings.dart |
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.
|
I just looked into per-task delegates and I think they'd also work for this. Any hooks the task delegate doesn't override go to the session-level delegate, so things like client certificates would still work. It'd avoid the limitations with websockets and redirect tracking, so maybe that's the way to go. I'll give it a try later. |
|
Hi @mertalev this is very high quality but I'll need a while to review it.
If I understand correctly, the current approach can't be made to honor max-redirects, right? But will the redirect handler attached to the session be called? Also, I guess that you are doing something like: final sharedSession = URLSession.sessionWithConfiguration(
..., onFinishedDownloading: onFinished);
void foo() {
sharedSession.downloadTaskWithRequest(...);
}
void bar() { // Possibly in another isolate
final client = CupertinoClient.fromSharedSession(sharedSession);
client.
}The @liamappelbe is there a way to generate Dart bindings for Swift-only methods like these: https://developer.apple.com/documentation/foundation/urlsession/bytes(from:delegate:)?language=objc |
In theory you could run this through swiftgen, and it would take care of generating an |
Yup, the Dart delegate vs Dart delegate vs task-level delegate: As for usage, I create a session in
Probably yes! |
|
I moved the legacy path to use Dart with |
3 participants
This PR enables sharing of a single URLSession with platform and across isolates. The existing implementation's delegate approach tightly couples a session to a single isolate. As a solution, it adds an implementation based on the
bytes(for:)API, which does not require delegate-based callbacks. This allows users to bring their own session configured with their own delegate. It enables my goal to use a single shared session in my app; this is optimal for multiplexing and caching and means a single place to configure authentication like client certificates.I've tested these changes in the app and made sure the conformance tests are passing with this implementation. A few changes were needed in the tests because
bytes(for:)does its own chunking internally before yielding bytes. I have not tested its performance. I'd guess it performs better since the implementation is more contained in Swift with less Dart trampolining/bookkeeping, but it'd be good to profile it.From a maintenance standpoint, I think this implementation is simpler than the delegate-based approach and might be able to replace it at some point, perhaps when the minimum iOS version is bumped to 15? It's relatively self-contained as it is, though, and should have no effect on existing users of cupertino_http.
Fixes #1002
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.