feat(ferry): add IsolateClient #405

knaeckeKami · 2022-09-14T16:16:16Z

added IsolateClient, which runs queries in another isolate in order to avoid jank on expensive normalization

This should solve all remaining issues regarding jank.

Also added example on how to use it in pokemon_explorer.

TODOs:

Features
- ☑ request
- ☑ readQuery
- ☑ writeQuery
- ☑ readFragment
- ☑ writeFragment
- ☑ evict dataId from cache
- ☑ retain on cache
- ☑ release from cache
- ☑ cache garbage collection
- ☑ enable custom message handling for features like token refresh, error reporting etc
Tests
- ☑ IsolateClient integration test across isolates
- ☑ handleCommand tests for unit testing the ferry isolate side logic
Documentation
- ☑ Comments
- ☑ Website
- ☐ Better Naming?
- ☑ Example on how to do token refresh over isolates
Correctness
- ☐ Verify all ports and streams are properly closed when not needed anymore
  - ☑ Verified request() is cancelled on Isolate when main cancels stream
  - ☑ Verified request() is cancelled on isolate when main disposed link

This is almost ready to be merged now.

I'd appreciate comments, suggestions for improvements, and any feedback.

@GP4cK @2shrestha22 @ValentinVignal

GP4cK · 2022-09-18T01:23:41Z

packages/ferry/lib/ferry_isolate.dart

@@ -0,0 +1,145 @@
+import 'dart:async';
+import 'dart:isolate';


Since dart:isolate is not supported on web, will this be an issue if we try to use this package in a web app? Should it be a separate package?

It will fail if you try to use the IsolateClient, yes. The standard client will work though. But yes, it worth considering it moving to another package.

GP4cK · 2022-09-18T01:52:20Z

packages/ferry/lib/src/isolate/isolate_commands.dart

+import 'package:ferry/ferry.dart';
+
+abstract class IsolateCommand {
+  SendPort sendPort;


We shouldn't have to add a sendPort to each command because we could use the sendPort that we receive when the isolate is created, right? (_IsolateInit#sendPort)

Since there can be many multiple Streams and one-off request active at the same time, using just one SendPort would make the communication more complicated, since we would have to track which Message belongs to which request.

It would certainly be possible if we gave every IsolateCommand instance a unique ID for example, but still - this makes the code way simpler IMO. Also in cases where we need to send the information that the main isolate has stopped listening to a stream.

It would be possible by using request IDs for example. However, according to the the flutter team, creating ReceivePorts
is pretty cheap performance wise ( https://discord.com/channels/608014603317936148/608016344520196173/1019723108283850762 ). so I am not sure if it is worth it.

I see. Sounds fair.

GP4cK · 2022-09-18T01:55:42Z

examples/pokemon_explorer/lib/src/client_isolate.dart

+Future<Client> _initClientIsolate(Map<String, dynamic>? params, SendPort? sendPort) async {
+  // don't use Hive.initFlutter to avoid dealing with method channels in the isolate
+  // instead, call getApplicationDocumentsDirectory() on the main isolate
+  // and pass the result to the ferry isolate
+  Hive.init(params!["hivePath"]);


We may as well make params required if we expect a hivePath and a url.
Actually, it should be a class rather than a Map.

Changed to Map to be non-nullable now.

Good point. Currently, the signature of the function that creates a the client on the new Isolate must be

typedef InitClient = Future<TypedLinkWithCache> Function( Map<String, dynamic> params, SendPort? sendMessageToMessageHandler, );

do you think we should make it generic, like

typedef InitClient<InitParams> = Future<TypedLinkWithCache> Function( InitParmams params, SendPort? sendMessageToMessageHandler, );

and change the create() method of the IsolateClient to

static Future<IsolateClient> create<InitParams>(InitClient<InitParams> initClient, {InitParams params, void Function(Object?)? messageHandler, IsolateSpawn? isolateSpawn})

This would be typesafe, which is definitely a big plus.

However, it could lead to users trying the send objects which cannot be serialized over isolates (like SharedPreferences, HiveBoxes, Closures... basically anything that is not just data), while Map<String, dynamic> is already typically used for json-like data.

Well, Isolate.spawn is generic so I'd say it's fair to do the same.
We can always add an API comment with /// to say "Make sure InitParams is serializable as it will be passed to the isolate".

True. I'll update it.

GP4cK · 2022-09-18T02:03:38Z

packages/ferry/lib/src/isolate/handle_command.dart

-      final disposeCommand = command as DisposeCommand;
-      disposeCommand.sendPort.send(null);
-      Isolate.exit(disposeCommand.sendPort);
+void handleCommand(


A more SOLID approach would be to add an abstract method called handler to the IsolateCommand class.

This way you would force all the implementing classes to implement this method and you wouldn't need this big switch/case or the CommandType enum.
In the future, if we need to add a new command, we will just create a new class that extends IsolateCommand and we wouldn't have to touch other parts of the code.

I agree that this would be the right way to do it usually, not sure in this specific instance though.

I didn't really look into the serialization format the dart uses when sending objects through isolates, but wouldn't that lead to overhead? Since the commands are created on the main isolate, then serialized, sent to the ferry isolate, and then deserialized to an IsolateCommand object again, the commands should probably be as lightweight as possible.

So to achieve a SOLID approach without sacrificing performance we would need to maintain a parallel class structure on the ferry isolate side, which I feel is overkill as long as there are only a handful of commands.

You're right.

packages/ferry/test/isolate/isolate_client_test.dart

…tead of a Map<String,dynamic>

- ferry@0.11.1 - ferry_exec@0.1.6 - ferry_flutter@0.6.1+2 - ferry_generator@0.6.0+1 - ferry_cache@0.6.2+1 - ferry_test_graphql@0.1.5-dev.4

Martin Kamleithner added 12 commits September 14, 2022 18:13

feat(ferry): add IsolateClient

292170e

chore(pokemon_explorer): format

b871dff

feat(ferry): add more methods to IsolateClient

4005856

feat(ferry): refactor IsolateClient, add more commands

0fc2a6f

feat(ferry): more IsolateClient features

d5363b9

chore(ferry): format

1b15ae8

feat(ferry): refactor single response commands

8376820

docs(ferry): add documentation for IsolateClient in website

086c831

test(ferry): add tests

2e53420

chore(ferry): format

e701d3d

refactor(ferry): cleanup request response communication

7972d25

chore(ferry): format

c96eaa9

knaeckeKami marked this pull request as ready for review September 18, 2022 00:40

Martin Kamleithner added 2 commits September 18, 2022 02:54

refactor(ferry): refactore handleCommand

e29c6df

refactor(ferry): refactore handleCommand

d58ebef

GP4cK reviewed Sep 18, 2022

View reviewed changes

docs(ferry): add example for token refresh over isolate

685db93

knaeckeKami force-pushed the feat/ferry_isolate branch from dec0985 to 685db93 Compare September 18, 2022 02:27

Martin Kamleithner added 6 commits September 18, 2022 04:44

refactor(ferry): make params map non null in pokemon example

4458c88

chore(ferry): format

4d5b490

chore(ferry): remove ignored lint, add unawaited

ccc13ed

refactor(ferry): use a generic type parameter for the init params ins…

944d1b1

…tead of a Map<String,dynamic>

docs(ferry): mention isolates in README

25cb800

chore(release): publish packages

d481ebb

- ferry@0.11.1 - ferry_exec@0.1.6 - ferry_flutter@0.6.1+2 - ferry_generator@0.6.0+1 - ferry_cache@0.6.2+1 - ferry_test_graphql@0.1.5-dev.4

knaeckeKami merged commit 35a434c into master Sep 18, 2022

knaeckeKami deleted the feat/ferry_isolate branch September 18, 2022 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ferry): add IsolateClient #405

feat(ferry): add IsolateClient #405

knaeckeKami commented Sep 14, 2022 •

edited

Loading

GP4cK Sep 18, 2022

knaeckeKami Sep 18, 2022

GP4cK Sep 18, 2022

knaeckeKami Sep 18, 2022

GP4cK Sep 18, 2022

GP4cK Sep 18, 2022

knaeckeKami Sep 18, 2022 •

edited

Loading

GP4cK Sep 18, 2022

knaeckeKami Sep 18, 2022

GP4cK Sep 18, 2022

knaeckeKami Sep 18, 2022

GP4cK Sep 18, 2022

feat(ferry): add IsolateClient #405

feat(ferry): add IsolateClient #405

Conversation

knaeckeKami commented Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knaeckeKami Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knaeckeKami commented Sep 14, 2022 •

edited

Loading

knaeckeKami Sep 18, 2022 •

edited

Loading