Skip to content

feat: support aborting HTTP requests #1773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 49 commits into from
Jul 8, 2025

Conversation

JaffaKetchup
Copy link
Contributor

@JaffaKetchup JaffaKetchup commented May 20, 2025

Resolves #424
Closes #978, closes #521, closes #602, closes #819

As discussed in #424, this adds an Abortable interface for BaseRequests that provides an abortTrigger which Clients may use to abort requests.

We can do it this time :D


  • 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.

Implemented `Abortable` interface for all available `BaseRequest`s
Added abort support to `IOClient`
WIP: abort support for `BrowserClient`
@brianquinlan brianquinlan self-requested a review May 20, 2025 21:57
@brianquinlan
Copy link
Collaborator

The outline looks good so far! Let me know if you need me to press the button to run the workflows.

Copy link

github-actions bot commented May 20, 2025

Package publishing

Package Version Status Publish tag (post-merge)
package:cronet_http 1.4.0-wip WIP (no publish necessary)
package:cupertino_http 2.2.0 already published at pub.dev
package:http 1.5.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

github-actions bot commented May 20, 2025

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 💚 93 % ⬆️ 2 %
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 💚 88 % ⬆️ 2 %
pkgs/http/lib/src/mock_client.dart 💚 100 %
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

Improved documentation
Updated CHANGELOG
@JaffaKetchup JaffaKetchup changed the title [WIP] feat: support abortion of in-flight HTTP requests feat: support abortion of HTTP requests May 25, 2025
@JaffaKetchup JaffaKetchup marked this pull request as ready for review May 25, 2025 13:39
@JaffaKetchup
Copy link
Contributor Author

Sorry @brianquinlan :D Hopefully the workflows should be good to go now (except the tests which I've commented above).

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.

Amazing work. I have a few nits and I'd like to patch this in and try it out myself before we merge it.

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.

I fixed all of the cronet_http fails and most of cupertino_http failures. I think that the ok_http failures are unrelated (and the cupertino_http failures are probably unrelated too). I'll take a closer look at the cupertino_http failures tomorrow.

@brianquinlan
Copy link
Collaborator

brianquinlan commented Jul 1, 2025

I think that the package:cupertino_http tests will be fixed in #1786 and the package:cronet_http test failures are unrelated.

@JaffaKetchup
Copy link
Contributor Author

JaffaKetchup commented Jul 2, 2025

@brianquinlan Not sure where/how to test the retry client effectively. Would just plugging it into the existing client conformance tests for io & html be acceptable? I'm not really sure why it is tested alone in the first place.

  testAbort(clientFactory(),
      supportsAbort: supportsAbort,
      canStreamRequestBody: canStreamRequestBody,
      canStreamResponseBody: canStreamResponseBody);
+ testAbort(RetryClient(clientFactory()),
+     supportsAbort: supportsAbort,
+     canStreamRequestBody: canStreamRequestBody,
+     canStreamResponseBody: canStreamResponseBody);

I think that it makes sense use the conformance tests on RetryClient but that test should be in pkg/http. May have test/http_retry_conformance_test.dart?

But that test would not check that the behavior if the request is aborted after the first request fails. Maybe you needs some tests in http_retry_test.dart like:

  test('', () async {
    var count = 0;
    final client = RetryClient(
        MockClient(expectAsync1((request) async {
          count++;
          if (count < 2) return Response('', 503);
          throw RequestAbortedException(...);
        }, count: 2)),
        delay: (_) => Duration.zero);

    expect(client.get(Uri.http('example.org', ''), throwsA<RequestAbortedException>(...));
    
  });

You should probably also add a test for the case where the Client does not support being aborted. Do you want to not retry is the request has been imported even if the Client does not throw RequestAborted? Like:

  test('', () async {
    var count = 0;
    final client = RetryClient(
        MockClient(expectAsync1((request) async {
          count++;
          if (count == 2) {
             abortCompleter.complete();
          }
          return Response('', 503);
        }, count: 2)),
        delay: (_) => Duration.zero);

    final response = await client.get(Uri.http('example.org', ''));
    expect(client.send(<abortable request with abortCompleter>), throwsA<RequestAbortedException>(...));
  });

@JaffaKetchup
Copy link
Contributor Author

@brianquinlan That should be all sorted, ready for re-review.

@brianquinlan
Copy link
Collaborator

Thanks for fixing the lints!


# TODO(brianquinlan): Remove this when a release version of `package:http`
# supports abortable requests.
dependency_overrides:
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but we could also address this by moving this repo to a workspace.


* Added support for aborting requests before they complete.
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉indeed 😂

/// Whether to send credentials such as cookies or authorization headers for
/// cross-site requests.
///
/// Defaults to `false`.
bool withCredentials = false;

bool _isClosed = false;
final _openRequestAbortControllers = <AbortController>[];
Copy link
Member

Choose a reason for hiding this comment

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

This might want a type on the left ('DO type annotate fields and top-level variables if the type isn't obvious'). 'Obvious' isn't precisely defined, but adding one would also match w/ the other fields.

Suggested change
final _openRequestAbortControllers = <AbortController>[];
final List<AbortController> _openRequestAbortControllers = [];

Copy link
Member

Choose a reason for hiding this comment

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

I really don't like this style. Prefer as-is!

Devon: No offence to your bad ideas. 😛

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @kevmoo here - since the type is clearly indicated on the RHS, adding it explicitly is redundant

// This occurs at `pipe`, which automatically closes the request once the
// request stream has been pumped in.
//
// Therefore, we have multiple strategies:
Copy link
Member

Choose a reason for hiding this comment

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

👍 - good docs on the various possible states

Co-authored-by: Devon Carew <[email protected]>
@brianquinlan
Copy link
Collaborator

I'm going to merge this. My intent is to release a 1.5.0-beta version and update the associated issues asking for testing.

@brianquinlan brianquinlan merged commit a4f5a8d into dart-lang:master Jul 8, 2025
48 of 51 checks passed
@JaffaKetchup
Copy link
Contributor Author

Sounds good to me. Thanks for all the help!

@brianquinlan
Copy link
Collaborator

Sounds good to me. Thanks for all the help!

@JaffaKetchup No, thank you for all the great work!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jul 10, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

http (https://github.com/dart-lang/http/compare/7d2d87e..4209e84):
  4209e84  2025-07-09  Brian Quinlan  [cronet_http] Upgrade to kotlin 1.8.10 (dart-lang/http#1789)
  d2886a0  2025-07-08  dependabot[bot]  Bump subosito/flutter-action in the github-actions group (dart-lang/http#1784)
  4097219  2025-07-08  Brian Quinlan  Prepare to release 1.5.0-beta (dart-lang/http#1787)
  a4f5a8d  2025-07-08  Luka S  feat: support aborting HTTP requests (dart-lang/http#1773)

i18n (https://github.com/dart-lang/i18n/compare/42c4932..c45e050):
  c45e0504  2025-07-09  Googler  No public description
  bdb94c25  2025-07-09  Googler  No public description

tools (https://github.com/dart-lang/tools/compare/6282b35..fd7cc89):
  fd7cc89a  2025-07-09  Nate Biggs  Fix wasm dry run event and tests (dart-lang/tools#2128)
  1500539b  2025-07-09  Liam Appelbe  [coverage] Expose `filterIgnored` function (dart-lang/tools#2123)
  99634476  2025-07-08  Nate Biggs  Add wasm dry run event to unified analytics. (dart-lang/tools#2125)

web (https://github.com/dart-lang/web/compare/fb8a149..354e229):
  354e229  2025-07-09  Nikechukwu  [interop] Add Support for Typealiases (dart-lang/web#407)
  f0f0914  2025-07-08  Nikechukwu  [interop] Add Support for Enums (dart-lang/web#404)

Change-Id: Ia691d4ca8d20ccb1d7a96598c369dabaf9124850
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/439804
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
@JaffaKetchup
Copy link
Contributor Author

JaffaKetchup commented Jul 10, 2025

(Just to note, this is now used by default in the latest version of flutter_map, so hopefully it all goes well :D)

@brianquinlan
Copy link
Collaborator

(Just to note, this is now used by default in the latest version of flutter_map, so hopefully it all goes well :D)

Awesome! I was wondering if we would get adequate testing for the beta release but I don't need to worry anymore ;-)

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.

Design an API to support aborting requests
4 participants