Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RPC] Delay sending messages when there are handlers running initialization #13180

Merged
merged 12 commits into from
Jan 16, 2024

Conversation

jfaltermeier
Copy link
Contributor

@jfaltermeier jfaltermeier commented Dec 18, 2023

What it does

With this PR ClientProxyHandlers will report their init state. When there is a running initialization, we delay sending messages on all handlers until this initialization is done.
This should prevent cases where messages sent later via an already initialized handler can overtake earlier messages that are waiting for their handler's initialization to finish.

This will help avoiding race condition like in #13172

Fixes #13158
Fixes #13172

Commits should be squashed when closing the PR

How to test

Follow-ups

Review checklist

Reminder for reviewers

@jfaltermeier jfaltermeier force-pushed the jf/13172_proxyHandlerInit branch from 047e404 to 5d35e32 Compare December 18, 2023 10:34
@jfaltermeier jfaltermeier changed the title [RPC] Initialize dependent proxies at same time WIP [RPC] Initialize dependent proxies at same time Jan 3, 2024
* add an event to listen for init events on rpc proxy handler
* eager initialize DOCUMENTS_EXT Proxy when EDITORS_AND_DOCUMENTS_EXT
was initialized
* automate init by specifying required remote depednencies on set
* add other dependencies
* make sure to not start initialization multiple times
@jfaltermeier jfaltermeier force-pushed the jf/13172_proxyHandlerInit branch 2 times, most recently from 20cce47 to 276b7e7 Compare January 3, 2024 10:46
* don't throw error on possible race condition but log only
* initialize dependencies before resolving promise
@jfaltermeier jfaltermeier force-pushed the jf/13172_proxyHandlerInit branch from 276b7e7 to 966d462 Compare January 3, 2024 11:03
@martin-fleck-at martin-fleck-at self-requested a review January 3, 2024 12:51
* pause communication during init of handlers
* introduce init callback allowing to implement this blocking mechanism
* fix initial init promise state
@jfaltermeier jfaltermeier changed the title WIP [RPC] Initialize dependent proxies at same time WIP [RPC] Delay sending messages when there are handlers running inittialization Jan 9, 2024
@jfaltermeier jfaltermeier changed the title WIP [RPC] Delay sending messages when there are handlers running inittialization [RPC] Delay sending messages when there are handlers running initialization Jan 9, 2024
@jfaltermeier
Copy link
Contributor Author

@martin-fleck-at Could you please have another look? I've added an init callback that delays already initialized handlers from sending their messages immediately while an init is going on. As far as I could see, this should sync the messages/events as expected.

@martin-fleck-at
Copy link
Contributor

@jfaltermeier Sure, I'll have another look at it tomorrow.

@jfaltermeier jfaltermeier marked this pull request as ready for review January 10, 2024 13:50
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me and I think it should properly sync messages during initialization phase. However, I do have a few challenges with the naming as I believe it makes it hard to understand what is happening. Please see inline comments for that.

packages/plugin-ext/src/common/proxy-handler.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/common/proxy-handler.ts Outdated Show resolved Hide resolved
* rename from init callback to proxy synchronizer
* use rpc promise to report init done
* rename from init callback to proxy synchronizer
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jfaltermeier jfaltermeier merged commit 6cd31cb into master Jan 16, 2024
14 checks passed
@jfaltermeier jfaltermeier deleted the jf/13172_proxyHandlerInit branch January 16, 2024 07:28
@github-actions github-actions bot added this to the 1.46.0 milestone Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
2 participants