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.

Sign up for GitHub

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

Jump to bottom

Set FusedTransformer as default #2250

Merged
merged 14 commits into from
Jun 18, 2024
Merged

Set FusedTransformer as default #2250

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b710ead
set FusedTransformer as default Transformer
knaeckeKami Jun 17, 2024
cdbaa74
fix: respect ResponseType.plain even when Content-Type is application…
knaeckeKami Jun 17, 2024
82a61f1
docs: describe FusedTransformer as default
knaeckeKami Jun 17, 2024
5df26cc
Merge branch 'main' of github.com:cfug/dio into default_transformer
knaeckeKami Jun 17, 2024
2b6422d
document 50 KiB isolate spawning threshold on FusedTransformer
knaeckeKami Jun 17, 2024
1f18fca
fmt
knaeckeKami Jun 17, 2024
eb4fb77
fix lints
knaeckeKami Jun 17, 2024
c59bb4e
reorder CHANGELOG entries
knaeckeKami Jun 17, 2024
a3465ea
add trailing comma
knaeckeKami Jun 17, 2024
0a0c4aa
remove unnecessary empty check on utf8 code path
knaeckeKami Jun 17, 2024
e0e3626
fmt
knaeckeKami Jun 17, 2024
1470066
flip condition check
knaeckeKami Jun 18, 2024
3fa8bdf
Update dio/lib/src/dio_mixin.dart
knaeckeKami Jun 18, 2024
97f25b3
trailing commas
knaeckeKami Jun 18, 2024
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
1 change: 1 addition & 0 deletions dio/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ See the [Migration Guide][] for the complete breaking changes list.**
- Split the Web implementation to `package:dio_web_adapter`.
- Add FusedTransformer for improved performance when decoding JSON.
- Improves `InterceptorState.toString()`.
- Set FusedTransformer as the default transformer.
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved

## 5.4.3+1

Expand Down
3 changes: 2 additions & 1 deletion dio/lib/src/dio_mixin.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ abstract class DioMixin implements Dio {
/// The default [Transformer] that transfers requests and responses
/// into corresponding content to send.
@override
Transformer transformer = BackgroundTransformer();
Transformer transformer =
FusedTransformer(contentLengthIsolateThreshold: 50 * 1024);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining this specific number? Is it chosen rather arbitrarily or an educated guess?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to be configurable?

Copy link
Contributor Author

@knaeckeKami knaeckeKami Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

50 * 1024 is the hardcoded default in BackgroundTransformer, I chose the same to keep the behavior as close as possible to the old one.
I did not measure how long it takes to decode on smartphones with the new approach. I copied the rationale for the 50 KiB threshold from BackgroundTransformer now.

IMO it makes sense to make it configurable, for example, when running the whole HTTP client in a background isolate already, users might not care about blocking the isolate for a few milliseconds, so they could set a higher (or -1 to disable spawning new isolates completely).

knaeckeKami marked this conversation as resolved.
Show resolved Hide resolved

bool _closed = false;

Expand Down
21 changes: 15 additions & 6 deletions dio/lib/src/transformers/fused_transformer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,16 @@ class FusedTransformer extends Transformer {
}

final isJsonContent = Transformer.isJsonMimeType(
responseBody.headers[Headers.contentTypeHeader]?.first,
);
responseBody.headers[Headers.contentTypeHeader]?.first,
) &&
responseType == ResponseType.json;

final customResponseDecoder = options.responseDecoder;

// No custom decoder was specified for the response,
// and the response is json -> use the fast path decoder
if (isJsonContent && customResponseDecoder == null) {
return _fastUtf8JsonDecode(responseBody);
return _fastUtf8JsonDecode(options, responseBody);
}
final responseBytes = await consolidateBytes(responseBody.stream);

Expand All @@ -88,9 +89,12 @@ class FusedTransformer extends Transformer {
if (isJsonContent && decodedResponse != null) {
// slow path decoder, since there was a custom decoder specified
return jsonDecode(decodedResponse);
} else if (decodedResponse != null) {
} else if (customResponseDecoder != null) {
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved
return decodedResponse;
} else {
if (responseBytes.isEmpty) {
Copy link
Member

Choose a reason for hiding this comment

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

Any references?

Copy link
Contributor Author

@knaeckeKami knaeckeKami Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, did not mean to commit that. Is unnecessary, utf8.decode returns an empty string anyway on an empty bytes list. removed.

return '';
}
// If the response is not JSON and no custom decoder was specified,
// assume it is an utf8 string
return utf8.decode(
Expand All @@ -100,7 +104,8 @@ class FusedTransformer extends Transformer {
}
}

Future<Object?> _fastUtf8JsonDecode(ResponseBody responseBody) async {
Future<Object?> _fastUtf8JsonDecode(
RequestOptions options, ResponseBody responseBody) async {
final contentLengthHeader =
responseBody.headers[Headers.contentLengthHeader];

Expand All @@ -111,6 +116,10 @@ class FusedTransformer extends Transformer {
// of the response or the length of the eagerly decoded response bytes
final int contentLength;

// Successful HEAD requests don't have a response body, even if the content-length header
// present.
final contentLengthHeaderImpliesContent = options.method != 'HEAD';
Copy link
Member

Choose a reason for hiding this comment

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

  • With CONNECT too although it's an edge case?
  • Also the MDN reference would be helpful to be placed here.
  • Could the name be neverHasResponseBody and then flip all conditions to the opposite?
final neverHasResponseBody = options.method == 'CONNECT' || options.method == 'HEAD';

if (neverHasResponseBody || !hasContentLengthHeader) {
  // ...
}

Copy link
Contributor Author

@knaeckeKami knaeckeKami Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue only occurs when the ResponseType is .json, the response body has a Content-Type of application/json, and a Content-Length header with a value > 0, but no response body.

A valid CONNECT response will have no Content-Type and no Content-Length header, so the transformer won't try to parse it as json.

A server MUST NOT send any Transfer-Encoding or Content-Length header fields in a 2xx (Successful) response to CONNECT. A client MUST ignore any Content-Length or Transfer-Encoding header fields received in a successful response to CONNECT.

I mean sure, we could also support invalid responses to CONNECT, but I'd argue if a user want's that, they should just set the ResponseType to .plain, .bytes or .stream. or use their own decoder / transformer.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for the explanation. But the flag should be flipped because if you are using its opposite result always you should simply flip it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true. done


// The eagerly decoded response bytes
// which is set if the content length is not specified and
// null otherwise (we'll feed the stream directly to the decoder in that case)
Expand All @@ -119,7 +128,7 @@ class FusedTransformer extends Transformer {
// If the content length is not specified, we need to consolidate the stream
// and count the bytes to determine if we should use an isolate
// otherwise we use the content length header
if (!hasContentLengthHeader) {
if (!hasContentLengthHeader || !contentLengthHeaderImpliesContent) {
responseBytes = await consolidateBytes(responseBody.stream);
contentLength = responseBytes.length;
} else {
Expand Down
33 changes: 33 additions & 0 deletions dio/test/transformer_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,21 @@ void main() {
expect(response, 'plain text');
});

test('ResponseType.plain takes precedence over content-type', () async {
final transformer = FusedTransformer();
final response = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.plain),
ResponseBody.fromString(
'{"text": "plain text"}',
200,
headers: {
Headers.contentTypeHeader: ['application/json'],
},
),
);
expect(response, '{"text": "plain text"}');
});

test('transformResponse handles streams', () async {
final transformer = FusedTransformer();
final response = await transformer.transformResponse(
Expand Down Expand Up @@ -254,6 +269,24 @@ void main() {
);
expect(request, '{"foo":"bar"}');
});

test(
'HEAD request with content-length but empty body should not return null',
() async {
final transformer = FusedTransformer();
final response = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json, method: 'HEAD'),
ResponseBody(
Stream.value(Uint8List(0)),
200,
headers: {
Headers.contentTypeHeader: ['application/json'],
Headers.contentLengthHeader: ['123'],
},
),
);
expect(response, null);
});
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved
});

group('consolidate bytes', () {
Expand Down
Loading