Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,13 @@ class NetworkRequestOverviewView extends StatelessWidget {
];
}

Widget _buildTimingRow(Color color, String label, Duration duration) {
final flex = (duration.inMicroseconds / data.duration!.inMicroseconds * 100)
Widget _buildTimingRow(
Color color,
String label,
Duration duration,
Duration totalDuration,
) {
final flex = (duration.inMicroseconds / totalDuration.inMicroseconds * 100)
.round();
return Flexible(
flex: flex,
Expand All @@ -721,7 +726,10 @@ class NetworkRequestOverviewView extends StatelessWidget {

Widget _buildHttpTimeGraph() {
final data = this.data as DartIOHttpRequestData;
if (data.duration == null || data.instantEvents.isEmpty) {
final requestDuration = data.duration;
if (requestDuration == null ||
requestDuration.inMicroseconds == 0 ||
data.instantEvents.isEmpty) {
return Container(
key: httpTimingGraphKey,
height: 18.0,
Expand All @@ -743,14 +751,18 @@ class NetworkRequestOverviewView extends StatelessWidget {
final timingWidgets = <Widget>[];
for (final instant in data.instantEvents) {
final duration = instant.timeRange.duration;
timingWidgets.add(_buildTimingRow(nextColor(), instant.name, duration));
timingWidgets.add(
_buildTimingRow(nextColor(), instant.name, duration, requestDuration),
);
}
final duration = Duration(
microseconds:
data.endTimestamp!.microsecondsSinceEpoch -
data.instantEvents.last.timestamp.microsecondsSinceEpoch,
);
timingWidgets.add(_buildTimingRow(nextColor(), 'Response', duration));
timingWidgets.add(
_buildTimingRow(nextColor(), 'Response', duration, requestDuration),
);
return Row(key: httpTimingGraphKey, children: timingWidgets);
}

Expand Down
61 changes: 49 additions & 12 deletions packages/devtools_app/lib/src/shared/http/http_request_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,40 @@ class DartIOHttpRequestData extends NetworkRequest {

bool get _hasError => _request.request?.hasError ?? false;

DateTime? get _endTime =>
_hasError ? _request.endTime : _request.response?.endTime;
DateTime? get _endTime => (_hasError || _isCancelled)
? _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
// 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 (inProgress || !isValid) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method is now the same as it was before, revert to original code. Thanks!

return null;
}
return _endTime?.difference(_request.startTime);
}

/// Whether the request is safe to display in the UI.
Expand All @@ -156,7 +182,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,
},
Expand Down Expand Up @@ -227,11 +253,11 @@ 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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add dartdoc back

bool get inProgress =>
_hasError ? !_request.isRequestComplete : !_request.isResponseComplete;
bool get inProgress {
if (_isCancelled) return false;
return _request.endTime == null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we still be checking for an error here along with using isRequestComplete/isResponseComplete?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think inProgress should still consider error/request/response completion
Agreed. Updated inProgress to use request/response completion semantics (including the error path), with cancellation handled separately.

}

/// All instant events logged to the timeline for this HTTP request.
List<DartIOHttpInstantEvent> get instantEvents {
Expand Down Expand Up @@ -273,6 +299,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!);
Expand Down Expand Up @@ -301,12 +328,22 @@ 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';

if (_hasError) return 'Error';

final statusCode = _request.response?.statusCode;
if (statusCode != null) return statusCode.toString();

return null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is equivalent to return _request.response?.statusCode.toString() (the original code) because the entire expression will evaluate to null if response is null

}

@override
String get uri => _request.uri.toString();

bool get _isCancelled => _hasCancellationError || _hasCancellationEvent;

String? get responseBody {
if (_request is! HttpProfileRequest) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ abstract class CustomPointerScrollView extends BoxScrollView {
super.physics,
super.shrinkWrap,
super.padding,
super.scrollCacheExtent,
super.cacheExtent,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CC @elliette ; from the PR description:

Update CustomPointerScrollView to use cacheExtent so the project compiles with the current Flutter SDK

super.semanticChildCount,
super.dragStartBehavior,
this.customPointerSignalHandler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ TODO: Remove this section if there are not any updates.
## Network profiler updates

- Added a filter setting to hide HTTP-profiler socket data. [#9698](https://github.com/flutter/devtools/pull/9698)
- Improved HTTP request status classification in the Network tab to better distinguish cancelled, completed, and in-flight requests (for example, avoiding some cases where cancelled requests appeared as pending). [#9683](https://github.com/flutter/devtools/pull/9683)

## Logging updates

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,17 @@ void main() {
expect(requests.length, numRequests);
final httpRequests = requests.whereType<DartIOHttpRequestData>().toList();
for (final request in httpRequests) {
expect(request.duration, request.inProgress ? isNull : isNotNull);
expect(
request.duration,
request.inProgress || request.endTimestamp == null
? isNull
: isNotNull,
);
expect(request.general.length, greaterThan(0));
expect(httpMethods.contains(request.method), true);
expect(request.status, request.inProgress ? isNull : isNotNull);
if (request.inProgress) {
expect(request.status, isNull);
}
}

// Finally, call `clear()` and ensure the requests have been cleared.
Expand Down Expand Up @@ -205,15 +212,31 @@ void main() {

controller.setActiveFilter(query: 'status:Error');
expect(profile, hasLength(numRequests));
expect(controller.filteredData.value, hasLength(1));

controller.setActiveFilter(query: 's:101');
final errorCount = profile
.whereType<DartIOHttpRequestData>()
.where((request) => request.status == 'Error')
.length;
expect(controller.filteredData.value, hasLength(errorCount));

final firstStatus = profile
.whereType<DartIOHttpRequestData>()
.map((request) => request.status)
.whereType<String>()
.first;
final firstStatusCount = profile
.whereType<DartIOHttpRequestData>()
.where((request) => request.status == firstStatus)
.length;
controller.setActiveFilter(query: 's:$firstStatus');
expect(profile, hasLength(numRequests));
expect(controller.filteredData.value, hasLength(1));
expect(controller.filteredData.value, hasLength(firstStatusCount));

controller.setActiveFilter(query: '-s:Error');
expect(profile, hasLength(numRequests));
expect(controller.filteredData.value, hasLength(8));
expect(
controller.filteredData.value,
hasLength(numRequests - errorCount),
);

controller.setActiveFilter(query: 'type:json');
expect(profile, hasLength(numRequests));
Expand Down Expand Up @@ -253,11 +276,28 @@ void main() {

controller.setActiveFilter(query: '-status:error method:get');
expect(profile, hasLength(numRequests));
expect(controller.filteredData.value, hasLength(3));
final nonErrorGetCount = profile
.whereType<DartIOHttpRequestData>()
.where(
(request) =>
request.method.toLowerCase() == 'get' &&
request.status?.toLowerCase() != 'error',
)
.length;
expect(controller.filteredData.value, hasLength(nonErrorGetCount));

controller.setActiveFilter(query: '-status:error method:get t:http');
expect(profile, hasLength(numRequests));
expect(controller.filteredData.value, hasLength(2));
final nonErrorGetHttpCount = profile
.whereType<DartIOHttpRequestData>()
.where(
(request) =>
request.method.toLowerCase() == 'get' &&
request.status?.toLowerCase() != 'error' &&
request.type.toLowerCase() == 'http',
)
.length;
expect(controller.filteredData.value, hasLength(nonErrorGetHttpCount));
});

test('filterData hides tcp sockets via setting filter', () async {
Expand Down Expand Up @@ -341,6 +381,21 @@ void main() {
'statusCode': 200,
},
})!;
final request1CancelledWithStatusCode = HttpProfileRequest.parse({
...httpBaseObject,
'events': [
{
'timestamp': startTime + 100,
'event': 'Request cancelled by client',
},
],
'response': {
'startTime': startTime,
'endTime': null,
'redirects': [],
'statusCode': 200,
},
})!;
final request2Pending = HttpProfileRequest.parse({
...httpBaseObject,
'id': '102',
Expand Down Expand Up @@ -403,6 +458,30 @@ void main() {
},
);

test('latest request update wins over stale status for same id', () {
currentNetworkRequests.updateOrAddAll(
requests: [request1Done],
sockets: const [],
timelineMicrosOffset: 0,
);

final initialRequest =
currentNetworkRequests.getRequest('101')! as DartIOHttpRequestData;
expect(initialRequest.status, '200');
expect(initialRequest.status, isNot('Cancelled'));

currentNetworkRequests.updateOrAddAll(
requests: [request1CancelledWithStatusCode],
sockets: const [],
timelineMicrosOffset: 0,
);

final updatedRequest =
currentNetworkRequests.getRequest('101')! as DartIOHttpRequestData;
expect(updatedRequest.status, 'Cancelled');
expect(updatedRequest.inProgress, false);
});

test('clear', () {
final reqs = [request1Pending, request2Pending];
final sockets = [socketStats1Pending, socketStats2Pending];
Expand Down
Loading