Set FusedTransformer as default by knaeckeKami · Pull Request #2250 · cfug/dio

knaeckeKami · 2024-06-17T20:21:52Z

New Pull Request Checklist

I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I'm adding
I have updated the documentation (if necessary)
I have run the tests without failures
I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

Also fix three oversights in the transformer

Fix parsing content as JSON when Content-Type is like application/json but ResponseType == .plain
HEAD requests have an empty body, even when Content-Length is >0. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD . I did not take this into account. This is also fixed now.
Respect custom decoders returning null

…/json fix: do not throw on empty response bodies on HEAD requests when Content-Type is json

ueman

Thanks again! I have no objections to changing the default transformer, but I'd like to hear the opinions on it from the others

ueman · 2024-06-17T20:29:16Z

  @override
-  Transformer transformer = BackgroundTransformer();
+  Transformer transformer =
+      FusedTransformer(contentLengthIsolateThreshold: 50 * 1024);


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

Does it make sense to be configurable?

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).

AlexV525 · 2024-06-17T23:08:28Z

+    } else if (customResponseDecoder != null) {
      return decodedResponse;
    } else {
+      if (responseBytes.isEmpty) {


Any references?

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

AlexV525 · 2024-06-17T23:11:38Z

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


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) { // ... }

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.

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.

Co-authored-by: Alex Li <github@alexv525.com> Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

github-actions · 2024-06-18T12:31:45Z

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
dio/lib/src/transformers/fused_transformer.dart	🟢 84.09%	🟢 91.3%	🟢 7.21%
dio/lib/src/transformers/sync_transformer.dart	🟢 100%	🟢 84.21%	🔴 -15.79%
Overall Coverage	🟢 82.45%	🟢 82.48%	🟢 0.03%

Minimum allowed coverage is 0%, this run produced 82.48%

AlexV525

fixes #2279 This improves handling of the following case: - ResponseType is json - Server Content-Type is json - Server Content-Length is nonzero - Response Body is empty This can happen in some cases according to HTTP spec (e.g. HEAD requests), but some servers return responses like this (see #2279) also in other cases, even if it violates the HTTP spec. With #2250, in some of these cases dio would throw an exception, which caused a regression for some users; see #2279 I do believe that dio should handle these cases for gracefully, This PR brings back the previous behavior of returning null. ### New Pull Request Checklist - [x] I have read the [Documentation](https://pub.dev/documentation/dio/latest/) - [x] I have searched for a similar pull request in the [project](https://github.com/cfug/dio/pulls) and found none - [x] I have updated this branch with the latest `main` branch to avoid conflicts (via merge from master or rebase) - [x] I have added the required tests to prove the fix/feature I'm adding - [x] I have updated the documentation (if necessary) - [x] I have run the tests without failures - [x] I have updated the `CHANGELOG.md` in the corresponding package ### Additional context and info (if any)  --------- Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com> Co-authored-by: Jonas Uekötter <ueman@users.noreply.github.com>

knaeckeKami added 4 commits June 17, 2024 21:13

set FusedTransformer as default Transformer

b710ead

fix: respect ResponseType.plain even when Content-Type is application…

cdbaa74

…/json fix: do not throw on empty response bodies on HEAD requests when Content-Type is json

docs: describe FusedTransformer as default

82a61f1

Merge branch 'main' of github.com:cfug/dio into default_transformer

5df26cc

knaeckeKami requested a review from a team as a code owner June 17, 2024 20:21

ueman reviewed Jun 17, 2024

View reviewed changes

knaeckeKami added 3 commits June 17, 2024 21:42

document 50 KiB isolate spawning threshold on FusedTransformer

2b6422d

fmt

1f18fca

fix lints

eb4fb77

kuhnroyal reviewed Jun 17, 2024

View reviewed changes

Comment thread dio/CHANGELOG.md

reorder CHANGELOG entries

c59bb4e

AlexV525 reviewed Jun 17, 2024

View reviewed changes

knaeckeKami added 4 commits June 18, 2024 00:55

add trailing comma

a3465ea

remove unnecessary empty check on utf8 code path

0a0c4aa

fmt

e0e3626

flip condition check

1470066

AlexV525 reviewed Jun 18, 2024

View reviewed changes

Comment thread dio/lib/src/dio_mixin.dart Outdated

knaeckeKami and others added 2 commits June 18, 2024 13:00

Update dio/lib/src/dio_mixin.dart

3fa8bdf

Co-authored-by: Alex Li <github@alexv525.com> Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

trailing commas

97f25b3

AlexV525 approved these changes Jun 18, 2024

View reviewed changes

ueman approved these changes Jun 18, 2024

View reviewed changes

ueman merged commit 47abf18 into cfug:main Jun 18, 2024

knaeckeKami mentioned this pull request Aug 18, 2024

fix: improve fallback to null on empty responses #2285

Merged

7 tasks

coderabbitai Bot mentioned this pull request Mar 20, 2026

Feature: Memory Organization Tools YKDBontekoe/miru#246

Open

Uh oh!

Uh oh!

Conversation

knaeckeKami commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Pull Request Checklist

Additional context and info (if any)

Uh oh!

ueman left a comment

Choose a reason for hiding this comment

Uh oh!

ueman Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

ueman Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexV525 Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexV525 Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexV525 Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 18, 2024

Code Coverage Report: Only Changed Files listed

Uh oh!

AlexV525 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knaeckeKami commented Jun 17, 2024 •

edited

Loading

knaeckeKami Jun 17, 2024 •

edited

Loading

knaeckeKami Jun 17, 2024 •

edited

Loading

knaeckeKami Jun 17, 2024 •

edited

Loading