Agent V2: fix #3434 by avoiding clone of WorkerBridge and WorkerProviderState

[deftsp]

Description

Fixes #3434

[github-actions]

Visit the preview URL for this PR (updated for commit 96649bc):

https://yew-rs-api--pr3435-fix-3434-aogu5kkl.web.app

_{(expires Mon, 09 Oct 2023 15:01:39 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[github-actions]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	102.835	102.835	0	0.000%
boids	175.606	175.606	0	0.000%
communication_child_to_parent	95.303	95.303	0	0.000%
communication_grandchild_with_grandparent	109.053	109.053	0	0.000%
communication_grandparent_to_grandchild	105.724	105.724	0	0.000%
communication_parent_to_child	92.792	92.792	0	0.000%
contexts	113.453	113.453	0	0.000%
counter	89.198	89.198	0	0.000%
counter_functional	89.934	89.934	0	0.000%
dyn_create_destroy_apps	92.317	92.317	0	0.000%
file_upload	103.517	103.517	0	0.000%
function_memory_game	174.576	174.576	0	0.000%
function_router	352.473	352.473	0	0.000%
function_todomvc	163.438	163.438	0	0.000%
futures	227.445	227.445	0	0.000%
game_of_life	112.166	112.166	0	0.000%
immutable	188.781	188.781	0	0.000%
inner_html	85.980	85.980	0	0.000%
js_callback	113.455	113.455	0	0.000%
keyed_list	201.204	201.204	0	0.000%
mount_point	89.187	89.187	0	0.000%
nested_list	114.608	114.608	0	0.000%
node_refs	96.290	96.290	0	0.000%
password_strength	1721.062	1721.062	0	0.000%
portals	98.363	98.363	0	0.000%
router	318.447	318.447	0	0.000%
simple_ssr	144.252	144.252	0	0.000%
ssr_router	390.238	390.238	0	0.000%
suspense	119.023	119.023	0	0.000%
timer	91.746	91.746	0	0.000%
timer_functional	100.451	100.451	0	0.000%
todomvc	143.686	143.686	0	0.000%
two_apps	89.896	89.896	0	0.000%
web_worker_fib	138.885	138.885	0	0.000%
web_worker_prime	190.365	190.365	0	0.000%
webgl	88.553	88.553	0	0.000%

✅ None of the examples has changed their size significantly.

[futursolo]

Thank you for the pull request.

After examining this issue, I think this bug came from the gloo-worker implementation, where the bridge should not be clonable.

E.g: Creating a bridge and cloning it 4 times then dropping all of them, worker will receive 1 connected message but 5 disconnected messages.

Cloning does not result in multiple handler id being assigned, which is not correct. We should not allow Bridges to be cloned. Users should use fork.

Would you like to file a pull request to gloo-worker to fix the underlying implementation?

[deftsp]

I found another problem, the register method of WorkerRegistrar be called for more than once for a single Worker. I think, one worker(like EventBus in my example) with many use_worker_bridge can only call the register once in a whole life. I'll figure it out soon.

[deftsp]

Thank you for the pull request.

After examining this issue, I think this bug came from the gloo-worker implementation, where the bridge should not be clonable.

E.g: Creating a bridge and cloning it 4 times then dropping all of them, worker will receive 1 connected message but 5 disconnected messages.

Cloning does not result in multiple handler id being assigned, which is not correct. We should not allow Bridges to be cloned. Users should use fork.

Would you like to file a pull request to gloo-worker to fix the underlying implementation?

if remove the Clone of WorkerBridge, only use fork. Then use_worker_bridge have to use fork, that means every render the fork will be called, that's expensive.

[futursolo]

if remove the Clone of WorkerBridge, only use fork. Then use_worker_bridge have to use fork, that means every render the fork will be called, that's expensive.

Why isn't using Rc<WorkerBridge<T>> an option?

[deftsp]

I think the Clone of WorkerBridge is fine. After removing the Clone of WorkerProviderState by using Rc<_>, everything works. Now, the Worker will not be destroyed, the connected', and disconnected` will be called only when the component is destroyed, not every render.

[futursolo]

Thank you! Looks good to me.

I am approving this pull request not because the underlying issue of gloo-worker doesn't need fixing any more but this pull request has moved away from WorkerBridge::clone so it is no longer an issue for Yew Agents.

[hamza1311]

I would argue that we should fix the underlying issue so gloo-worker consumers can also benefit from it. This PR's fix feels like a bandaid over a bigger issue

[deftsp]

I agree this PR covers the issue of gloo-worker. But wrapping the WorkerProviderState and WorkerBridge in Rc does not just fix the bug of #3434, but is more efficient. It is an optimization.

The yew_agent release version or the master branch should be kept workable, I think this PR should be merged before gloo-worker fix the issue.

gloo-worker I'll try to pull request later.

[hamza1311]

Gloo worker patch is also released: rustwasm/gloo#388 (comment)