-
Notifications
You must be signed in to change notification settings - Fork 394
Fix cancelled HTTP requests showing as Pending in DevTools Network tab #9685
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
Changes from 9 commits
45b1c2a
3ecd00f
d74f363
f5204f6
39babfa
2ad05f8
54eafea
0503e54
d9162b6
0f7a854
76cd92d
8a50293
5de8e82
d3d1ae7
8f8ed54
59d02dc
686699f
f5c8639
eafbdc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,11 +138,57 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| DateTime? get _endTime => | ||
| _hasError ? _request.endTime : _request.response?.endTime; | ||
|
|
||
| static const _cancellationMarkers = [ | ||
|
srawlins marked this conversation as resolved.
Outdated
srawlins marked this conversation as resolved.
Outdated
|
||
| 'cancel', | ||
| 'canceled', | ||
| 'cancelled', | ||
| 'operation canceled', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are just searching in a String for these values, we either don't need the longer ones ("operation canceled") or we don't need the shorter ones ("canceled"). I think it is find to just keep the shorter ones. Are these values derived from some general libraries, like the dio package and dart:io?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, they’re derived from cancellation wording observed in common Dart HTTP stacks, specifically our dart:io, package:http, and dio companion flows and the resulting VM profile payload strings. |
||
| 'operation cancelled', | ||
| 'abort', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should remove the shortest two in this list, "cancel" and "abort." Unless there are examples from dart:io, package:http, or package:dio, where a cancelled error/event contains "cancel" or "abort" but not "cancelled"/"canceled" or "aborted."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestions, I’ve updated things accordingly. I kept just canceled, cancelled, and aborted, and removed the longer phrases like operation canceled / operation cancelled since we’re already doing substring matching. I also dropped the more generic terms like cancel and abort to avoid false positives. These markers are based on the cancellation wording we’ve seen in related flows (like dart:io request.abort(), package:http client.close(), and dio cancelToken.cancel(...)) along with the profiler payload text. One thing to note: in companion cancel mode, we can still get a 200 since the status gets set early so in those cases, classification relies on explicit cancellation text in the error/event payloads. |
||
| 'aborted', | ||
| ]; | ||
|
|
||
| bool _matchesCancellationMarker(String? value) { | ||
| final normalized = value?.toLowerCase(); | ||
| if (normalized == null) return false; | ||
|
srawlins marked this conversation as resolved.
Outdated
|
||
| return _cancellationMarkers.any(normalized.contains); | ||
| } | ||
|
|
||
| bool get _hasCancellationError { | ||
| final requestError = _request.request?.error; | ||
| final responseError = _request.response?.error; | ||
| return _matchesCancellationMarker(requestError) || | ||
| _matchesCancellationMarker(responseError); | ||
| } | ||
|
|
||
| bool get _hasCancellationEvent => | ||
| _request.events.any((event) => _matchesCancellationMarker(event.event)); | ||
|
|
||
| @override | ||
| Duration? get duration { | ||
| if (inProgress || !isValid) return null; | ||
| // Timestamps are in microseconds | ||
| return _endTime!.difference(_request.startTime); | ||
| if (_hasError) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are these changes for? It looks like the |
||
| final start = _request.startTime; | ||
| final end = _request.endTime; | ||
| return end?.difference(start); | ||
| } | ||
|
|
||
| // Cancelled request | ||
|
srawlins marked this conversation as resolved.
Outdated
|
||
| if (isCancelled) { | ||
| return Duration.zero; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it could still be beneficial to include timing information even if the request was canceled - this matches what Chrome DevTools network panel does. |
||
| } | ||
|
|
||
| if (inProgress) { | ||
| return null; | ||
| } | ||
|
|
||
| final start = _request.startTime; | ||
| final end = _request.response?.endTime ?? _request.endTime; | ||
|
|
||
| if (end != null) { | ||
| return end.difference(start); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /// Whether the request is safe to display in the UI. | ||
|
|
@@ -156,7 +202,7 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| return { | ||
| 'method': _request.method, | ||
| 'uri': _request.uri.toString(), | ||
| if (!didFail) ...{ | ||
| if (!didFail && !isCancelled) ...{ | ||
| 'connectionInfo': _request.request?.connectionInfo, | ||
| 'contentLength': _request.request?.contentLength, | ||
| }, | ||
|
|
@@ -227,11 +273,19 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| return connectionInfo != null ? connectionInfo[_localPortKey] : null; | ||
| } | ||
|
|
||
| /// True if the HTTP request hasn't completed yet, determined by the lack of | ||
| /// an end time in the response data. | ||
| @override | ||
|
srawlins marked this conversation as resolved.
|
||
| bool get inProgress => | ||
| _hasError ? !_request.isRequestComplete : !_request.isResponseComplete; | ||
| bool get inProgress { | ||
| if (isCancelled) { | ||
| return false; | ||
| } | ||
|
|
||
| final statusCode = _request.response?.statusCode; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand why adding the |
||
| if (statusCode != null) { | ||
| return false; | ||
| } | ||
|
|
||
| return _request.endTime == null; | ||
|
rishika0212 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /// All instant events logged to the timeline for this HTTP request. | ||
| List<DartIOHttpInstantEvent> get instantEvents { | ||
|
|
@@ -273,6 +327,7 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| bool get didFail { | ||
| if (status == null) return false; | ||
| if (status == 'Error') return true; | ||
| if (status == 'Cancelled') return false; | ||
|
|
||
| try { | ||
| final code = int.parse(status!); | ||
|
|
@@ -301,12 +356,36 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| DateTime get startTimestamp => _request.startTime; | ||
|
|
||
| @override | ||
| String? get status => | ||
| _hasError ? 'Error' : _request.response?.statusCode.toString(); | ||
| String? get status { | ||
| if (isCancelled) return 'Cancelled'; | ||
|
|
||
| final statusCode = _request.response?.statusCode; | ||
| if (statusCode != null) return statusCode.toString(); | ||
|
|
||
| if (_hasError) return 'Error'; | ||
|
srawlins marked this conversation as resolved.
|
||
|
|
||
| return null; | ||
| } | ||
|
|
||
| @override | ||
| String get uri => _request.uri.toString(); | ||
|
|
||
| bool get isCancelled { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the only reason this is public is to expose it for tests. Can we instead make this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, agreed. I made it private and updated tests to assert via status instead of exposing isCancelled just for testing |
||
| if (_hasCancellationError || _hasCancellationEvent) { | ||
| return true; | ||
| } | ||
|
|
||
| if (_request.request?.error != null && _request.response == null) { | ||
| return true; | ||
| } | ||
|
|
||
| if (_request.endTime != null && _request.response == null) { | ||
| return true; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are these checks for? It looks like these are checks for whether the request is in progress but I might be misunderstanding something here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those checks were trying to catch cancelled requests when we don’t get a normal response, but I agree they also blurred into in-progress/completed logic. I removed them and kept cancellation detection focused on explicit cancellation signals, with completion handled separately |
||
|
|
||
| return false; | ||
| } | ||
|
|
||
| String? get responseBody { | ||
| if (_request is! HttpProfileRequest) { | ||
| return null; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] pull
data.durationinto a named variable (e.g.requestDuration) so that we don't need the!null assertion operator (data.duration!)