feat: support aborting HTTP requests #1773
Conversation
Implemented `Abortable` interface for all available `BaseRequest`s Added abort support to `IOClient` WIP: abort support for `BrowserClient`
|
The outline looks good so far! Let me know if you need me to press the button to run the workflows. |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR Health
Breaking changes
|
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| cronet_http | Breaking | 1.3.4 | 1.4.0-wip | 2.0.0 Got "1.4.0-wip" expected >= "2.0.0" (breaking changes) |
|
| cupertino_http | Breaking | 2.2.0 | 2.2.0 | 3.0.0 Got "2.2.0" expected >= "3.0.0" (breaking changes) |
|
| http | Breaking | 1.4.0 | 1.5.0-wip | 2.0.0 Got "1.5.0-wip" expected >= "2.0.0" (breaking changes) |
|
| ok_http | Breaking | 0.1.0 | 0.1.1-wip | 0.2.0 Got "0.1.1-wip" expected >= "0.2.0" (breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Changelog Entry ❗
| Package | Changed Files |
|---|---|
| package:cronet_http | pkgs/cronet_http/example/pubspec.yaml |
| package:cupertino_http | pkgs/cupertino_http/example/pubspec.yaml |
| package:ok_http | pkgs/ok_http/example/pubspec.yaml |
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/http.dart | 💚 100 % |
| pkgs/http/lib/retry.dart | 💔 85 % ⬇️ 7 % |
| pkgs/http/lib/src/abortable.dart | 💚 100 % |
| pkgs/http/lib/src/base_request.dart | 💚 88 % |
| pkgs/http/lib/src/browser_client.dart | 💔 Not covered |
| pkgs/http/lib/src/io_client.dart | 💚 87 % ⬆️ 1 % |
| pkgs/http/lib/src/multipart_request.dart | 💔 93 % ⬇️ 4 % |
| pkgs/http/lib/src/request.dart | 💚 98 % ⬆️ 0 % |
| pkgs/http/lib/src/streamed_request.dart | 💚 100 % |
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 symbols |
|---|---|
| cupertino_http | ncb.NSURLCache NSURLRequest NSURLRequestAttribution NSCachedURLResponse ncb.NSURLSessionDelegate ncb.NSURLSessionConfiguration SSLProtocol tls_protocol_version_t NSHTTPCookieStorage NSURLCredentialStorage __CFRunLoopObserver __CFRunLoopTimer __SecTrust __CFError __darwin_mcontext64 _opaque_pthread_attr_t __darwin_sigaltstack __darwin_ucontext __siginfo sigval rusage_info_v6 _malloc_zone_t _opaque_pthread_cond_t _opaque_pthread_condattr_t _opaque_pthread_mutex_t _opaque_pthread_mutexattr_t _opaque_pthread_once_t _opaque_pthread_rwlock_t _opaque_pthread_rwlockattr_t _opaque_pthread_t UnsignedWide ProcessSerialNumber Point Rect wide TimeBaseRecord NumVersionVariant NumVersion VersRec Float80 Float96 __CFString __CFNull __CFAllocator __CFArray __SecCertificate __SecIdentity __SecKey __SecPolicy __SecAccessControl __SecKeychain __SecKeychainItem __SecKeychainSearch SecKeychainAttribute __SecTrustedApplication __SecAccess __SecACL __SecPassword __sFILE __CFBag __CFBinaryHeap __CFBitVector __CFDictionary __CFNotificationCenter __CFLocale __CFDate __CFTimeZone __CFData __CFCharacterSet __CFCalendar __CFDateFormatter __CFBoolean __CFNumber __CFNumberFormatter __CFURL mach_port_status mach_port_limits mach_port_info_ext mach_port_guard_info mach_port_qos mach_service_port_info mach_port_options UnnamedUnion1 __CFRunLoop __CFRunLoopSource __CFSocket fsignatures fsupplement fchecklv fgetsigsinfo fstore fpunchhole ftrimactivefile fspecread fattributiontag _filesec OS_os_workgroup os_workgroup_attr_opaque_s os_workgroup_join_token_opaque_s os_workgroup_max_parallel_threads_attr_s os_workgroup_interval_data_opaque_s time_value mach_timespec mach_msg_mac_trailer_t security_token_t audit_token_t msg_labels_t mach_msg_security_trailer_t dispatch_source_type_s __CFReadStream __CFWriteStream __CFSet __CFTree __CFUUID __CFBundle __CFMessagePort __CFPlugInInstance __CFMachPort __CFAttributedString __CFURLEnumerator kauth_ace guid_t kauth_acl kauth_filesec _acl _acl_entry _acl_permset _acl_flagset __CFFileSecurity __CFStringTokenizer __CFFileDescriptor __CFUserNotification __CFXMLNode __CFXMLParser cssm_data SecAsn1Template_struct cssm_guid cssm_version cssm_subservice_uid cssm_net_address cssm_crypto_data cssm_list_element UnnamedUnion2 cssm_list CSSM_TUPLE cssm_tuplegroup cssm_sample cssm_samplegroup cssm_memory_funcs cssm_encoded_cert cssm_parsed_cert cssm_cert_pair cssm_certgroup UnnamedUnion3 cssm_base_certs cssm_access_credentials cssm_authorizationgroup cssm_acl_validity_period cssm_acl_entry_prototype cssm_acl_owner_prototype cssm_acl_entry_input cssm_resource_control_context cssm_acl_entry_info cssm_acl_edit cssm_func_name_addr cssm_date cssm_range cssm_query_size_data cssm_key_size cssm_keyheader cssm_key cssm_dl_db_handle cssm_context_attribute cssm_context_attribute_value cssm_kr_profile cssm_context cssm_pkcs1_oaep_params cssm_csp_operational_statistics cssm_pkcs5_pbkdf1_params cssm_pkcs5_pbkdf2_params cssm_kea_derive_params cssm_tp_authority_id cssm_field cssm_tp_policyinfo cssm_dl_db_list cssm_tp_callerauth_context cssm_encoded_crl cssm_parsed_crl cssm_crl_pair cssm_crlgroup UnnamedUnion4 cssm_fieldgroup cssm_evidence cssm_tp_verify_context cssm_tp_verify_context_result cssm_tp_request_set cssm_tp_result_set cssm_tp_confirm_response cssm_tp_certissue_input cssm_tp_certissue_output cssm_tp_certchange_input cssm_tp_certchange_output cssm_tp_certverify_input cssm_tp_certverify_output cssm_tp_certnotarize_input cssm_tp_certnotarize_output cssm_tp_certreclaim_input cssm_tp_certreclaim_output cssm_tp_crlissue_input cssm_tp_crlissue_output cssm_cert_bundle_header cssm_cert_bundle cssm_db_attribute_info cssm_db_attribute_label cssm_db_attribute_data cssm_db_record_attribute_info cssm_db_record_attribute_data cssm_db_parsing_module_info cssm_db_index_info cssm_db_unique_record cssm_db_record_index_info cssm_dbinfo cssm_selection_predicate cssm_query_limits cssm_query cssm_dl_pkcs11_attributes cssm_name_list cssm_db_schema_attribute_info cssm_db_schema_index_info SecAsn1AlgId cssm_x509_type_value_pair cssm_x509_rdn cssm_x509_name SecAsn1PubKeyInfo cssm_x509_time x509_validity cssm_x509ext_basicConstraints cssm_x509_extensionTagAndValue cssm_x509ext_pair cssm_x509_extension cssm_x509ext_value extension_data_format cssm_x509_extensions cssm_x509_tbs_certificate cssm_x509_signature cssm_x509_signed_certificate cssm_x509ext_policyQualifierInfo cssm_x509ext_policyQualifiers cssm_x509ext_policyInfo cssm_x509_revoked_cert_entry cssm_x509_revoked_cert_list cssm_x509_tbs_certlist cssm_x509_signed_crl __CE_OtherName __CE_GeneralName __CE_GeneralNameType __CE_GeneralNames __CE_AuthorityKeyID __CE_ExtendedKeyUsage __CE_BasicConstraints __CE_PolicyQualifierInfo __CE_PolicyInformation __CE_CertPolicies __CE_DistributionPointName UnnamedUnion5 __CE_CrlDistributionPointNameType __CE_CRLDistributionPoint __CE_CRLDistPointsSyntax __CE_AccessDescription __CE_AuthorityInfoAccess __CE_SemanticsInformation __CE_QC_Statement __CE_QC_Statements __CE_IssuingDistributionPoint __CE_GeneralSubtree __CE_GeneralSubtrees __CE_NameConstraints __CE_PolicyMapping __CE_PolicyMappings __CE_PolicyConstraints __CE_DataAndType CE_Data __CE_DataType cssm_acl_process_subject_selector cssm_acl_keychain_prompt_selector cssm_appledl_open_parameters cssm_applecspdl_db_settings_parameters cssm_applecspdl_db_is_locked_parameters cssm_applecspdl_db_change_password_parameters SSLContext |
| ok_http | $Certificate$NullableType $Certificate$Type PublicKey $PublicKey$NullableType $PublicKey$Type $PublicKey $X509Certificate$NullableType $X509Certificate$Type $PrivateKey$NullableType $PrivateKey$Type $PrivateKey |
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 |
…medRequest` respects `Abortable` presence)
Improved documentation
Integrated abort tests
Improved documentation
Updated CHANGELOG
JaffaKetchup
changed the title
pkgs/http_client_conformance_tests/lib/http_client_conformance_tests.dart
Outdated
Show resolved
Hide resolved
|
Sorry @brianquinlan :D Hopefully the workflows should be good to go now (except the tests which I've commented above). |
pkgs/http_client_conformance_tests/lib/http_client_conformance_tests.dart
Outdated
Show resolved
Hide resolved
|
I can't seem to reproduce either of those two failures. I can reproduce the first one only when using a
|
|
The Do we need to do that? |
|
You can see both failures in this CI run: https://github.com/dart-lang/http/actions/runs/15307371338/job/43064107042?pr=1773 |
|
I'm not sure why it would be working on mine and failing on CI - I guess CI is just 30 seconds slower which somehow causes the issue. How would we cancel the request stream? Put it through another internal controller, and inject an error into it on abortion, which I think will make |
I haven't looked into how request cancellation in |
|
I'm assuming the test is hanging because the input stream is never Unfortunately I've got more limited time coming up over the next couple weeks, so I'm happy to implement but don't really have too much time for experimentation. |
I'm on holiday for two weeks starting this Friday so I don't have a lot of time in the short term either. My intuition was that getting this to work reliably would be difficult, given that we have multiple implementations and cancellation can happen at any point in the request. Let me make a suggestion on #424 |
|
I can't be certain, but it looks like that fixed the issue? |
| final response = client.send(request); | ||
| request.sink.add('Hello World'.codeUnits); | ||
|
|
||
| abortTrigger.complete(); |
There was a problem hiding this comment.
Closing the sink will break if we change the code to:
expect(
response,
throwsA(isA<AbortedRequest>()),
);
for (int i = 0; i < 10000; ++i) {
request.sink.add('Hello World'.codeUnits);
await Future.delayed(const Duration());
}
Bad state: Cannot add event after closing
There was a problem hiding this comment.
So my suggestion would be to either:
- make this a forbidden action (the user shouldn't be trying to pipe in after aborting)
- make a dummy consumer which silently switches away from the aborted request and drops incoming data
There was a problem hiding this comment.
So my suggestion would be to either:
- make this a forbidden action (the user shouldn't be trying to pipe in after aborting)
That will make the API quite hard to use safely.
- make a dummy consumer which silently switches away from the aborted request and drops incoming data
The implementation of pipe is simple:
return streamConsumer.addStream(this).then((_) => streamConsumer.close());
I'm surprised that HttpClientRequest.abort doesn't cancel the stream.
There was a problem hiding this comment.
The docs for abort say:
/// Using the [IOSink] methods (e.g., [write] and [add]) has no effect after
/// the request has been aborted
There was a problem hiding this comment.
I believe addStream is 'uninteruptable'. I guess moving away from pipe and doing it manually might give more flexibility.
There was a problem hiding this comment.
Is the user expected to close the input stream when streaming normal requests, or is that done internally by finalised?
There was a problem hiding this comment.
Normally the user would have to close the sink, otherwise the consumer can't know when the end of data has been reached.
So my test is probably bad. But I'm trying the proxy approach so that the stream subscription will be cancelled if the request is aborted.
There was a problem hiding this comment.
I think that I changed my mind about this. Since streaming uploads are not supported by BrowserClient, it will never cancel the stream subscription. So I don't think that we need to do so in IOClient.
There was a problem hiding this comment.
Ok, so should I revert my change and you can update the tests, or are you working on it?
There was a problem hiding this comment.
I did update the tests but I think that I screwed something up. I'm now getting a null assertion failure in SDK code:
void _returnConnection(_HttpClientConnection connection) { _connectionTargets[connection.key]!.returnConnection(connection);
_connectionsChanged();
}which is why this test is failing:
00:03 +53 ~1 -1: test/io/client_test.dart: detachSocket returns a socket from an IOStreamedResponse [E]
|
|
I think that the problem is that we now call |
|
This change seems to fix the flake: 9001a1d |
| /// | ||
| /// This exception is triggered when [Abortable.abortTrigger] completes. | ||
| /// property for more info. | ||
| class AbortedRequest extends ClientException { |
There was a problem hiding this comment.
Has there been discussion on naming here?
Generally we'd follow AbortedRequestException or similar naming, right?
There was a problem hiding this comment.
Or maybe RequestAbortedException?
There was a problem hiding this comment.
AbortedRequest sounds like a "flavor" or Request, which this certainly is NOT.
There was a problem hiding this comment.
There is not been a discussion on naming. +1 to changing the exception name.
There was a problem hiding this comment.
I can see the potential confusion that it looks like an object that holds information about an aborted request. But the reason I was against calling it an exception (and against extending from client exception initially), is because it doesn't meet the docs on Exception:
...information ... about a failure
See also TickerCanceled: https://api.flutter.dev/flutter/scheduler/TickerCanceled-class.html
RequestAborted is better - but I wouldn't necessarily want to include "exception".
|
I added a (currently failing) test that verifies the behavior when the response stream is not being listened to when the response ends: |
github-actions
bot
added
package:cupertino_http
|
I don't have any more time to work on this today (and I'm not sure about tomorrow). But I think that we have an outline of a solution for the |
|
@JaffaKetchup I'm on holiday until June 23rd. If you need approval to run the github workflow, maybe ping @natebosch here? |
Resolves #424, closes #204, closes #424, closes #567
Closes #978, closes #521, closes #602, closes #819
As discussed in #424, this adds an
Abortableinterface forBaseRequeststhat provides anabortTriggerwhichClients may use to abort requests.We can do it this time :D
Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.