[Dart2] Add initial support to use an isolate to offload JSON serialization/deserialization

[noordawod]

I have noticed that as my API grew and its associated data requirements grew with it, a Flutter app I work on started showing hickups (jank) when a lengthy JSON hit the wire. So, I wanted to offload to a compute() function but was unable due to the fact that ApiClient._deserialize is private. Hence this pr.

In addition, I had to adjust the signatures of some functions and methods to be futures and that means that this pr is not backward-compatible. So if users have overridden serialize() or deserialize() then they'll need to update their code to match the new signature. A matter of 5 minutes but the change allows a better concurrency regardless if one uses an isolate or not.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master, 5.1.x, 6.0.x
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@ircecho (2017/07) @swipesight (2018/09) @jaumard (2018/09) @athornz (2019/12) @amondnet (2019/12) @wing328 @sbu-WBT @agilob

[agilob]

While technically this is backwards incompatible change, it doesn't feel like it. Good practice should be accepted.

[noordawod]

While technically this is backwards incompatible change, it doesn't feel like it. Good practice should be accepted.

That's great to hear! Thanks for the approval. I'll get the pr to pass all tests asap.

[agilob]

It's still up to @wing328 to decide whether this gets in 5.2 or 6.0, but for me it's a candidate for 5.2. Breaking change is minimal, but benefits are big.

[noordawod]

It's still up to @wing328 to decide whether this gets in 5.2 or 6.0, but for me it's a candidate for 5.2. Breaking change is minimal, but benefits are big.

I agree. Let's wait and see what @wing328 thinks. Hoping for 5.2!

[noordawod]

@agilob @wing328 tests are green! 🎉

[noordawod]

ps: after implementing the offloading via compute(), the app is significantly more responsive and the jank (even the slightest) is gone. I can truly say now that the app achieves 60 frames/sec with ease even with very high network load.

[agilob]

Nice, what do you think about creating a service with isolates for api client? apiClient would be wrapped in an isolate rather than Futures? Would it be better practice?

[noordawod]

I think this is beyond the scope of what the API client should be. One can always use it inside an isolate, but from my experience it's not needed at all. The heaviest operations in the client are JSON serialization/deserialization, and I think this pr would solve that bottleneck with minimal changes.

[agilob]

Can you add a comment that the synchronous method is deprecated and marked for removal in 6.0?

[noordawod]

Can you add a comment that the synchronous method is deprecated and marked for removal in 6.0?

There is a deprecation already, but I'll add the note about removal in 6.0.

[noordawod]

@agilob @wing328 @kuhnroyal
Can you guys take a look at the failing test and understand why it's happening? As far as I can see, I didn't change the tests at all -- just called the async versions serialize/deserialize.

[kuhnroyal]

You previous changes to the tests should have solved this and the test succeeds for me. The issue is causes by adding multiple pets to the FakeClient which internally now awaits the serialization but by that time the expected response has already been overwritten. Remember the test exists twice.

Although we can probably remove one of the tests (openapi-2).
Additionally the FakeClient tests also don't really test anything. They will succeed even if the serialization is wrong, since the client and the expectations both use the clients serialization fucntions.

[noordawod]

@wing328 @agilob what do you think?

[kuhnroyal]

Although we can probably remove one of the tests (openapi-2).
Additionally the FakeClient tests also don't really test anything. They will succeed even if the serialization is wrong, since the client and the expectations both use the clients serialization fucntions.

This should be done separately, for now you should just fix both tests in the same way you did earlier.
samples/openapi3/client/petstore/dart2/petstore/test/pet_faked_client_test.dart
samples/client/petstore/dart2/petstore/test/pet_faked_client_test.dart

[noordawod]

Although we can probably remove one of the tests (openapi-2).
Additionally the FakeClient tests also don't really test anything. They will succeed even if the serialization is wrong, since the client and the expectations both use the clients serialization fucntions.

This should be done separately, for now you should just fix both tests in the same way you did earlier.
samples/openapi3/client/petstore/dart2/petstore/test/pet_faked_client_test.dart
samples/client/petstore/dart2/petstore/test/pet_faked_client_test.dart

I rolled back my "fix", but can certainly bring it back. Do you think it'll solve the tests?

[kuhnroyal]

Yes I do and it did :)
You just forgot the openapi-2 tests last time.

[noordawod]

woo hoo! that was an awesome input, thank @kuhnroyal.

[noordawod]

What's left for merging? :)

[wing328]

@noordawod just merged. Thanks for the PR.

@agilob @kuhnroyal thanks for the review.

[vladaman]

@noordawod any chance to add support for null-safety and dart >= 2.12 ? Thanks!

[noordawod]

@vladaman I talked with @agilob a while ago about adding null-safety but I just don't have time right now. But definitely in the planning.