-
Notifications
You must be signed in to change notification settings - Fork 389
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 11 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,58 @@ class DartIOHttpRequestData extends NetworkRequest { | |
| DateTime? get _endTime => | ||
| _hasError ? _request.endTime : _request.response?.endTime; | ||
|
|
||
| bool _matchesCancellationMarker(String? value) { | ||
| if (value == null) return false; | ||
| final normalized = value.toLowerCase(); | ||
|
|
||
| /// Markers used for substring matching against request / response errors | ||
|
srawlins marked this conversation as resolved.
Outdated
|
||
| /// and request event names to classify cancelled requests. | ||
| /// | ||
| /// Derived from observed cancellation wording in HTTP profiler payloads, | ||
| /// keeping specific terms to reduce false positives. | ||
| const _cancellationMarkers = [ | ||
| 'canceled', | ||
| 'cancelled', | ||
| 'aborted', | ||
| ]; | ||
|
|
||
| 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); | ||
| } | ||
|
|
||
| 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 +203,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 +274,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 +328,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 +357,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!)