Worker v2 #200

futursolo · 2022-03-18T13:37:06Z

This pull request simplifies Workers to have 1 single type.

This consists of the following changes:

Unifies PublicWorker and PrivateWorker
Added a WorkerSpawner to supersede Worker::bridge() with a builder pattern to spawn workers. WorkerSpawner always spawn a new worker.
Removed WorkerLink and opted for WorkerScope directly.
WorkerBirdge::fork() which supports the former PublicWorker usage. It can "fork" the bridge with a different callback and HandlerId.

This pull request is feature complete.
However, I have modified some associated types on messages which could cause the main implementation of Worker to not be able to be tree shaked automatically. I will test this as I update the yew-agent crate and turn this into a pull request once I have the new agent implementation ready.

ranile · 2022-03-18T13:40:37Z

It would be nice if you could also add tests for worker implementation. Assuming that's possible, of course.

futursolo · 2022-03-18T13:47:20Z

It's actually relatively cumbersome to compile workers at the moment. However, the next version of trunk has introduced worker support which we may have a better chance after the release of the next version of Trunk.

droogmic · 2022-05-28T19:38:32Z

Can this be completed now that trunk supports workers?

droogmic · 2022-06-12T17:41:45Z

FYI: I had a problem moving to v2, because the resource path was missing this prefix:
https://github.com/droogmic/gloo/blob/master/crates/worker/src/worker/mod.rs#L85-L89

futursolo · 2022-06-12T23:13:52Z

FYI: I had a problem moving to v2, because the resource path was missing this prefix:
https://github.com/droogmic/gloo/blob/master/crates/worker/src/worker/mod.rs#L85-L89

I think this has been fixed in 64a08e0 as it now uses the URL of current page as base.
Could you please try again?

If it does not work, could you provide the URL of the page and the path of the worker that is passed to .spawn()?

droogmic · 2022-06-14T20:29:56Z

5a0bd6 looks incredible, thank you!

droogmic · 2022-06-14T20:36:22Z

FYI: I had a problem moving to v2, because the resource path was missing this prefix:
https://github.com/droogmic/gloo/blob/master/crates/worker/src/worker/mod.rs#L85-L89

I think this has been fixed in 64a08e0 as it now uses the URL of current page as base. Could you please try again?

If it does not work, could you provide the URL of the page and the path of the worker that is passed to .spawn()?

yup, this works, checked in a-b-street/osm2lanes@f1eff7d

Now I need to figure out how to integrate rkyv as an alternative encoding to bincode (bincode doesn't play nice with serde's flatten)

droogmic · 2022-06-14T20:50:05Z

5a0bd6 looks incredible, thank you!

ah, I spoke too soon. This still depends on serde. Maybe there is another serde-compatible binary coding crate that doesn't have the same problems.

futursolo · 2022-06-15T03:56:19Z

serde-compatible binary coding

You should be able to use serde_json or serde-wasm-bindgen.
The message format accepts any type that supports structured clone.

droogmic · 2022-06-16T10:31:42Z

Thanks @futursolo, I got this working end to end now here: a-b-street/osm2lanes#214

I will keep this updated with your changes.

# Conflicts: # crates/worker/src/worker/private.rs

ranile

Thanks for this PR and all the amazing work that you've done. Sorry it look me a little while to get to this.

Since this is a complete rewrite, I think we should have a migration guide (not exactly sure where it would go). What do you think? I think a page on the website or a markdown file in this directory would do the job.

ranile · 2022-06-21T19:59:35Z

crates/worker/src/codec.rs

+
+/// Default message encoding with [bincode].
+#[derive(Debug)]
+pub struct Bincode {}


Suggested change

pub struct Bincode {}

pub struct Bincode;

They have different semantics. The current implementation can't be created like:

let c = Bincode; // incorrect let c = Bincode {}; // correct

I think it's better to remove the { }. Adding fields will be a breaking change regardless.

You will never need to instantiate the Bincode struct.

It is supposed to be used with Worker::spawner().encoding::<Bincode>().spawn().

But I have removed it anyways.

ranile · 2022-06-21T20:02:37Z

crates/worker/src/codec.rs

+/// Default message encoding with [bincode].
+#[derive(Debug)]
+pub struct Bincode {}


Should bincode be the default, with no way to opt out? Postcard recently hit 1.0, which is no_std so it should have a smaller footprint in the binary. I think we should allow users to customize it. A simple feature flag (enabled by default) will be enough here. Those who want to customize can implement Codec manually

What do you think?

Should bincode be the default, with no way to opt out?

If we do not provide a default encoding, it means that users will have to specify Codec for every agent.
I don't think this would be a feature that most users would want to use. Assuming a default encoding would result in a simplified encoding.

Dead code elimination is effective in removing Bincode when it is not selected.

Those who want to customize can implement Codec manually

This is already possible by specifying a Codec other than Bincode. Any type that implements Codec can be passed to encoding() method of Spawner and Registrar.

ranile · 2022-06-21T20:22:53Z

crates/worker/src/native_worker.rs

+                };
+                let closure = Closure::wrap(Box::new(handler) as Box<dyn Fn(MessageEvent)>);
+                self.set_onmessage(Some(closure.as_ref().unchecked_ref()));
+                // Memory leak?


Yes. This is a memory leak, the closure is never freed.

Is it possible to store the closure somewhere? If this is a hot path, the memory leak(s) can add up. How many times will this be called?

I have converted it to use into_js_value().
If the bundle is built with weak reference support, it will be tracked by the browser's GC and will be destroyed after it's not used.

crates/worker/src/registrar.rs

ranile · 2022-06-21T20:33:59Z

examples/markdown/src/bin/example_markdown_app.rs

+        // We need to hold the bridge until the worker resolves.
+        let promise = Promise::new(&mut |_, _| {});
+        let _ = JsFuture::from(promise).await;


I don't really understand this part. Shouldn't it be instant? Or do we need the tick from the micro task queue, after the promise is resolved?

The application is getting data from a "thread".

The main thread will not be able to see the message until it is notified by a thread synchronisation mechanism(MessageChannel in this case). It will not be notified until control is handed back to the event loop.

examples/markdown/src/bin/example_markdown_test_server.rs

futursolo

I don't know where the migration guide should reside in either.

But as it is completely rewritten, it might be better to simply advise users to re-implement their agent like what's advised in the yew-router 0.16 migration guide.

futursolo · 2022-06-22T04:18:49Z

crates/worker/src/codec.rs

+
+/// Default message encoding with [bincode].
+#[derive(Debug)]
+pub struct Bincode {}


You will never need to instantiate the Bincode struct.

It is supposed to be used with Worker::spawner().encoding::<Bincode>().spawn().

But I have removed it anyways.

futursolo · 2022-06-22T04:38:26Z

crates/worker/src/codec.rs

+/// Default message encoding with [bincode].
+#[derive(Debug)]
+pub struct Bincode {}


Should bincode be the default, with no way to opt out?

If we do not provide a default encoding, it means that users will have to specify Codec for every agent.
I don't think this would be a feature that most users would want to use. Assuming a default encoding would result in a simplified encoding.

Dead code elimination is effective in removing Bincode when it is not selected.

Those who want to customize can implement Codec manually

This is already possible by specifying a Codec other than Bincode. Any type that implements Codec can be passed to encoding() method of Spawner and Registrar.

futursolo · 2022-06-22T04:52:32Z

crates/worker/src/native_worker.rs

+                };
+                let closure = Closure::wrap(Box::new(handler) as Box<dyn Fn(MessageEvent)>);
+                self.set_onmessage(Some(closure.as_ref().unchecked_ref()));
+                // Memory leak?


I have converted it to use into_js_value().
If the bundle is built with weak reference support, it will be tracked by the browser's GC and will be destroyed after it's not used.

futursolo · 2022-06-22T04:56:10Z

examples/markdown/src/bin/example_markdown_app.rs

+        // We need to hold the bridge until the worker resolves.
+        let promise = Promise::new(&mut |_, _| {});
+        let _ = JsFuture::from(promise).await;


The application is getting data from a "thread".

The main thread will not be able to see the message until it is notified by a thread synchronisation mechanism(MessageChannel in this case). It will not be notified until control is handed back to the event loop.

ranile

Thanks again for the PR!

huntc · 2022-06-23T09:07:48Z

Thanks also from me! I think this is a great improvement.

futursolo added 9 commits March 17, 2022 21:45

Move messages to a different file.

435f9d6

Remove WorkerLink.

3845128

Move WorkerExt to a different file.

ea8e5ec

Bridge, Spawner and Registrar.

7abcaf2

Implement Spawner.

fb8cf16

Remove existing implementation.

ff774ff

Remove more unused code.

defbbba

Remove more unused code.

950060e

Better naming.

e3aebf5

futursolo added 5 commits March 19, 2022 10:27

worker_ext -> native_worker.

2845afa

Registers the first callback as well.

a3eb5b9

Fix exports.

74d5d1a

Less worker prefixes.

a2ba75f

Restore Worker Prefix, PartialEq on WorkerBridge.

d26e873

ranile mentioned this pull request Mar 20, 2022

Agent Ergonomics: Box<dyn Bridge<A: Agent>> is offputting yewstack/yew#1406

Closed

3 tasks

Introduce a method to delay worker shutdown.

19974b7

droogmic mentioned this pull request May 28, 2022

Worker: support rkyv in additon to bincode #221

Closed

huntc mentioned this pull request Jun 11, 2022

Remove the private worker refcell #223

Merged

futursolo added 4 commits June 13, 2022 01:34

Fix message handling.

f292201

Merge branch 'master' into worker-v2

3130340

Fix URL handling.

64a08e0

Name WorkerScope scope.

3af881f

futursolo added 3 commits June 13, 2022 20:49

Add an example.

8ee5efa

Add test.

891ced8

Cargo fmt.

1d16320

futursolo marked this pull request as ready for review June 13, 2022 12:42

Swappable Encoding.

c5a0bd6

futursolo added 4 commits June 15, 2022 13:00

Adds an encoding setter for Registrar as well.

a3dbd0e

Adjust the worker signature so it matches Component v2.

1bbeff4

Workers should continue to receive to messages.

6843d99

More reliable destroy behaviour.

3952ebb

futursolo added 6 commits June 16, 2022 20:15

Adjust documentation & Destroy handle.

75402be

Export worker registrar.

ae2435b

Avoid panicking after worker is destroyed.

fa79c2c

Adjust documentation.

cc6968f

Make Registrable and Spawnable type configurable.

23a1e27

Merge branch 'master' into worker-v2

a2101e1

# Conflicts: # crates/worker/src/worker/private.rs

droogmic approved these changes Jun 18, 2022

View reviewed changes

ranile reviewed Jun 21, 2022

View reviewed changes

Instructions.

9a7b422

futursolo commented Jun 22, 2022

View reviewed changes

ranile approved these changes Jun 23, 2022

View reviewed changes

ranile merged commit 52572bd into rustwasm:master Jun 23, 2022

futursolo mentioned this pull request Jul 23, 2022

Send Connected notification for workers #183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Worker v2 #200

Worker v2 #200

futursolo commented Mar 18, 2022

ranile commented Mar 18, 2022

futursolo commented Mar 18, 2022

droogmic commented May 28, 2022

droogmic commented Jun 12, 2022

futursolo commented Jun 12, 2022

droogmic commented Jun 14, 2022

droogmic commented Jun 14, 2022

droogmic commented Jun 14, 2022

futursolo commented Jun 15, 2022

droogmic commented Jun 16, 2022

ranile left a comment

ranile Jun 21, 2022

futursolo Jun 22, 2022

ranile Jun 21, 2022

futursolo Jun 22, 2022

ranile Jun 21, 2022

futursolo Jun 22, 2022

ranile Jun 21, 2022

futursolo Jun 22, 2022

futursolo left a comment

futursolo Jun 22, 2022

futursolo Jun 22, 2022

futursolo Jun 22, 2022

futursolo Jun 22, 2022

ranile left a comment

huntc commented Jun 23, 2022

Worker v2 #200

Worker v2 #200

Conversation

futursolo commented Mar 18, 2022

ranile commented Mar 18, 2022

futursolo commented Mar 18, 2022

droogmic commented May 28, 2022

droogmic commented Jun 12, 2022

futursolo commented Jun 12, 2022

droogmic commented Jun 14, 2022

droogmic commented Jun 14, 2022

droogmic commented Jun 14, 2022

futursolo commented Jun 15, 2022

droogmic commented Jun 16, 2022

ranile left a 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

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

futursolo left a 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

ranile left a comment

Choose a reason for hiding this comment

huntc commented Jun 23, 2022