Skip to content
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

[package:http_profile] Store request and response bodies in the backing map as lists instead of as streams #1154

Merged
merged 1 commit into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 9 additions & 4 deletions pkgs/http_profile/lib/src/http_client_request_profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,15 @@ final class HttpClientRequestProfile {
_data['requestData'] = <String, dynamic>{};
requestData = HttpProfileRequestData._(_data, _updated);
_data['responseData'] = <String, dynamic>{};
responseData = HttpProfileResponseData._(
_data['responseData'] as Map<String, dynamic>, _updated);
_data['_requestBodyStream'] = requestData._body.stream;
_data['_responseBodyStream'] = responseData._body.stream;
responseData = HttpProfileResponseData._(_data, _updated);
_data['requestBodyBytes'] = <int>[];
requestData._body.stream.listen(
(final bytes) => (_data['requestBodyBytes'] as List<int>).addAll(bytes),
);
_data['responseBodyBytes'] = <int>[];
responseData._body.stream.listen(
(final bytes) => (_data['responseBodyBytes'] as List<int>).addAll(bytes),
);
// This entry is needed to support the updatedSince parameter of
// ext.dart.io.getHttpProfile.
_updated();
Expand Down
1 change: 1 addition & 0 deletions pkgs/http_profile/lib/src/http_profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:collection' show UnmodifiableListView;
import 'dart:developer' show Service, addHttpClientProfilingData;
import 'dart:io' show HttpClient, HttpClientResponseCompressionState;
import 'dart:isolate' show Isolate;
Expand Down
14 changes: 9 additions & 5 deletions pkgs/http_profile/lib/src/http_profile_request_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,13 @@ final class HttpProfileRequestData {
Map<String, dynamic> get _requestData =>
_data['requestData'] as Map<String, dynamic>;

/// The body of the request.
/// A sink that can be used to record the body of the request.
StreamSink<List<int>> get bodySink => _body.sink;

/// The body of the request represented as an unmodifiable list of bytes.
List<int> get bodyBytes =>
UnmodifiableListView(_data['requestBodyBytes'] as List<int>);

/// Information about the networking connection used in the HTTP request.
///
/// This information is meant to be used for debugging.
Expand Down Expand Up @@ -155,10 +159,10 @@ final class HttpProfileRequestData {
///
/// [endTime] is the time when the request was fully sent. It defaults to the
/// current time.
void close([DateTime? endTime]) {
Future<void> close([DateTime? endTime]) async {
_checkAndUpdate();
_isClosed = true;
unawaited(bodySink.close());
await bodySink.close();
_data['requestEndTimestamp'] =
(endTime ?? DateTime.now()).microsecondsSinceEpoch;
}
Expand All @@ -172,10 +176,10 @@ final class HttpProfileRequestData {
///
/// [endTime] is the time when the error occurred. It defaults to the current
/// time.
void closeWithError(String value, [DateTime? endTime]) {
Future<void> closeWithError(String value, [DateTime? endTime]) async {
_checkAndUpdate();
_isClosed = true;
unawaited(bodySink.close());
await bodySink.close();
_requestData['error'] = value;
_data['requestEndTimestamp'] =
(endTime ?? DateTime.now()).microsecondsSinceEpoch;
Expand Down
58 changes: 34 additions & 24 deletions pkgs/http_profile/lib/src/http_profile_response_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,23 @@ final class HttpProfileResponseData {
final void Function() _updated;
final StreamController<List<int>> _body = StreamController<List<int>>();

Map<String, dynamic> get _responseData =>
_data['responseData'] as Map<String, dynamic>;

/// Records a redirect that the connection went through.
void addRedirect(HttpProfileRedirectData redirect) {
_checkAndUpdate();
(_data['redirects'] as List<Map<String, dynamic>>).add(redirect._toJson());
(_responseData['redirects'] as List<Map<String, dynamic>>)
.add(redirect._toJson());
}

/// The body of the response.
/// A sink that can be used to record the body of the response.
StreamSink<List<int>> get bodySink => _body.sink;

/// The body of the response represented as an unmodifiable list of bytes.
List<int> get bodyBytes =>
UnmodifiableListView(_data['responseBodyBytes'] as List<int>);

/// Information about the networking connection used in the HTTP response.
///
/// This information is meant to be used for debugging.
Expand All @@ -57,7 +65,7 @@ final class HttpProfileResponseData {
);
}
}
_data['connectionInfo'] = {...value};
_responseData['connectionInfo'] = {...value};
}

/// The reponse headers where duplicate headers are represented using a list
Expand All @@ -74,10 +82,10 @@ final class HttpProfileResponseData {
set headersListValues(Map<String, List<String>>? value) {
_checkAndUpdate();
if (value == null) {
_data.remove('headers');
_responseData.remove('headers');
return;
}
_data['headers'] = {...value};
_responseData['headers'] = {...value};
}

/// The response headers where duplicate headers are represented using a
Expand All @@ -94,10 +102,10 @@ final class HttpProfileResponseData {
set headersCommaValues(Map<String, String>? value) {
_checkAndUpdate();
if (value == null) {
_data.remove('headers');
_responseData.remove('headers');
return;
}
_data['headers'] = splitHeaderValues(value);
_responseData['headers'] = splitHeaderValues(value);
}

// The compression state of the response.
Expand All @@ -107,57 +115,57 @@ final class HttpProfileResponseData {
// uncompressed bytes when they listen to the response body byte stream.
set compressionState(HttpClientResponseCompressionState value) {
_checkAndUpdate();
_data['compressionState'] = value.name;
_responseData['compressionState'] = value.name;
}

// The reason phrase associated with the response e.g. "OK".
set reasonPhrase(String? value) {
_checkAndUpdate();
if (value == null) {
_data.remove('reasonPhrase');
_responseData.remove('reasonPhrase');
} else {
_data['reasonPhrase'] = value;
_responseData['reasonPhrase'] = value;
}
}

/// Whether the status code was one of the normal redirect codes.
set isRedirect(bool value) {
_checkAndUpdate();
_data['isRedirect'] = value;
_responseData['isRedirect'] = value;
}

/// The persistent connection state returned by the server.
set persistentConnection(bool value) {
_checkAndUpdate();
_data['persistentConnection'] = value;
_responseData['persistentConnection'] = value;
}

/// The content length of the response body, in bytes.
set contentLength(int? value) {
_checkAndUpdate();
if (value == null) {
_data.remove('contentLength');
_responseData.remove('contentLength');
} else {
_data['contentLength'] = value;
_responseData['contentLength'] = value;
}
}

set statusCode(int value) {
_checkAndUpdate();
_data['statusCode'] = value;
_responseData['statusCode'] = value;
}

/// The time at which the initial response was received.
set startTime(DateTime value) {
_checkAndUpdate();
_data['startTime'] = value.microsecondsSinceEpoch;
_responseData['startTime'] = value.microsecondsSinceEpoch;
}

HttpProfileResponseData._(
this._data,
this._updated,
) {
_data['redirects'] = <Map<String, dynamic>>[];
_responseData['redirects'] = <Map<String, dynamic>>[];
}

void _checkAndUpdate() {
Expand All @@ -176,11 +184,12 @@ final class HttpProfileResponseData {
///
/// [endTime] is the time when the response was fully received. It defaults
/// to the current time.
void close([DateTime? endTime]) {
Future<void> close([DateTime? endTime]) async {
derekxu16 marked this conversation as resolved.
Show resolved Hide resolved
_checkAndUpdate();
_isClosed = true;
unawaited(bodySink.close());
_data['endTime'] = (endTime ?? DateTime.now()).microsecondsSinceEpoch;
await bodySink.close();
_responseData['endTime'] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this was incorrect before?

Copy link
Member Author

Choose a reason for hiding this comment

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

It worked before because we were awaiting _body.stream.expand(...).toList(...) on the SDK side, so it was impossible for this unawaited bodySink.close() to cause the 'requestBody' or 'responseBody' entries to get populated incorrectly here (https://github.com/dart-lang/sdk/blob/9efa95674966c1d8b498c1846372f36db689f8c7/sdk/lib/io/network_profiling.dart#L34-L44).

With the changes in this PR, we need to await bodySink.close() to guarantee that all the bytes that have been added to the sink have also been added to _data['requestBodyBytes'] before the SDK tries to read _data['requestBodyBytes'].

requestData._body.stream.listen(
(final bytes) => (_data['requestBodyBytes'] as List<int>).addAll(bytes),
);

(endTime ?? DateTime.now()).microsecondsSinceEpoch;
}

/// Signal that receiving the response has failed with an error.
Expand All @@ -192,11 +201,12 @@ final class HttpProfileResponseData {
///
/// [endTime] is the time when the error occurred. It defaults to the current
/// time.
void closeWithError(String value, [DateTime? endTime]) {
Future<void> closeWithError(String value, [DateTime? endTime]) async {
_checkAndUpdate();
_isClosed = true;
unawaited(bodySink.close());
_data['error'] = value;
_data['endTime'] = (endTime ?? DateTime.now()).microsecondsSinceEpoch;
await bodySink.close();
_responseData['error'] = value;
_responseData['endTime'] =
(endTime ?? DateTime.now()).microsecondsSinceEpoch;
}
}
16 changes: 8 additions & 8 deletions pkgs/http_profile/test/close_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ void main() {
group('requestData.close', () {
test('no arguments', () async {
expect(backingMap['requestEndTimestamp'], isNull);
profile.requestData.close();
await profile.requestData.close();

expect(
backingMap['requestEndTimestamp'],
Expand All @@ -39,7 +39,7 @@ void main() {

test('with time', () async {
expect(backingMap['requestEndTimestamp'], isNull);
profile.requestData.close(DateTime.parse('2024-03-23'));
await profile.requestData.close(DateTime.parse('2024-03-23'));

expect(
backingMap['requestEndTimestamp'],
Expand All @@ -48,7 +48,7 @@ void main() {
});

test('then write body', () async {
profile.requestData.close();
await profile.requestData.close();

expect(
() => profile.requestData.bodySink.add([1, 2, 3]),
Expand All @@ -57,7 +57,7 @@ void main() {
});

test('then mutate', () async {
profile.requestData.close();
await profile.requestData.close();

expect(
() => profile.requestData.contentLength = 5,
Expand All @@ -75,7 +75,7 @@ void main() {

test('no arguments', () async {
expect(responseData['endTime'], isNull);
profile.responseData.close();
await profile.responseData.close();

expect(
responseData['endTime'],
Expand All @@ -86,7 +86,7 @@ void main() {

test('with time', () async {
expect(responseData['endTime'], isNull);
profile.responseData.close(DateTime.parse('2024-03-23'));
await profile.responseData.close(DateTime.parse('2024-03-23'));

expect(
responseData['endTime'],
Expand All @@ -95,7 +95,7 @@ void main() {
});

test('then write body', () async {
profile.responseData.close();
await profile.responseData.close();

expect(
() => profile.responseData.bodySink.add([1, 2, 3]),
Expand All @@ -104,7 +104,7 @@ void main() {
});

test('then mutate', () async {
profile.responseData.close();
await profile.responseData.close();

expect(
() => profile.responseData.contentLength = 5,
Expand Down
16 changes: 14 additions & 2 deletions pkgs/http_profile/test/http_profile_request_data_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void main() {

test('populating HttpClientRequestProfile.requestEndTimestamp', () async {
expect(backingMap['requestEndTimestamp'], isNull);
profile.requestData.close(DateTime.parse('2024-03-23'));
await profile.requestData.close(DateTime.parse('2024-03-23'));

expect(
backingMap['requestEndTimestamp'],
Expand Down Expand Up @@ -91,7 +91,7 @@ void main() {
final requestData = backingMap['requestData'] as Map<String, dynamic>;
expect(requestData['error'], isNull);

profile.requestData.closeWithError('failed');
await profile.requestData.closeWithError('failed');

expect(requestData['error'], 'failed');
});
Expand Down Expand Up @@ -185,4 +185,16 @@ void main() {
expect(proxyDetails['isDirect'], true);
expect(proxyDetails['port'], 4321);
});

test('using HttpClientRequestProfile.requestData.bodySink', () async {
final requestBodyBytes = backingMap['requestBodyBytes'] as List<int>;
expect(requestBodyBytes, isEmpty);
expect(profile.requestData.bodyBytes, isEmpty);

profile.requestData.bodySink.add([1, 2, 3]);
await profile.requestData.close();

expect(requestBodyBytes, [1, 2, 3]);
expect(profile.requestData.bodyBytes, [1, 2, 3]);
});
}
27 changes: 9 additions & 18 deletions pkgs/http_profile/test/http_profile_response_data_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// 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.

import 'dart:async';
import 'dart:developer' show getHttpClientProfilingData;
import 'dart:io';

Expand Down Expand Up @@ -187,7 +186,7 @@ void main() {
final responseData = backingMap['responseData'] as Map<String, dynamic>;
expect(responseData['endTime'], isNull);

profile.responseData.close(DateTime.parse('2024-03-23'));
await profile.responseData.close(DateTime.parse('2024-03-23'));

expect(
responseData['endTime'],
Expand All @@ -199,28 +198,20 @@ void main() {
final responseData = backingMap['responseData'] as Map<String, dynamic>;
expect(responseData['error'], isNull);

profile.responseData.closeWithError('failed');
await profile.responseData.closeWithError('failed');

expect(responseData['error'], 'failed');
});

test('using HttpClientRequestProfile.requestBodySink', () async {
final requestBodyStream =
backingMap['_requestBodyStream'] as Stream<List<int>>;

profile.requestData.bodySink.add([1, 2, 3]);
profile.requestData.close();

expect(await requestBodyStream.expand((i) => i).toList(), [1, 2, 3]);
});
derekxu16 marked this conversation as resolved.
Show resolved Hide resolved

test('using HttpClientRequestProfile.responseBodySink', () async {
final responseBodyStream =
backingMap['_responseBodyStream'] as Stream<List<int>>;
test('using HttpClientRequestProfile.responseData.bodySink', () async {
final responseBodyBytes = backingMap['responseBodyBytes'] as List<int>;
expect(responseBodyBytes, isEmpty);
expect(profile.responseData.bodyBytes, isEmpty);

profile.responseData.bodySink.add([1, 2, 3]);
profile.responseData.close();
await profile.responseData.close();

expect(await responseBodyStream.expand((i) => i).toList(), [1, 2, 3]);
expect(responseBodyBytes, [1, 2, 3]);
expect(profile.responseData.bodyBytes, [1, 2, 3]);
});
}