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

Let Client.attach wait until stream initialization is finished #440

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

cozitive
Copy link
Contributor

@cozitive cozitive commented Jan 18, 2023

What this PR does / why we need it?

Client.attach can be terminated before getting peer presence map in WatchDocuments stream initialization. It can cause presence missing, which breaks eventual consistency, when updatePresence is called before the initialization.
This PR made Client.attach wait until the initialization is finished. Presence updates will be executed after peer presence map is properly set.

Any background context you want to provide?

If delay() is removed in the code below, test in the last line intermittently fails.

// Since attach's response doesn't wait for the watch ready,
// We need to wait here until the threshold.
await delay(100);
const unsub1 = c1.subscribe(spy1);
const unsub2 = c2.subscribe(spy2);
// Since `updatePresence` handles event publishing synchronously with
// Memory Coordinator, We need to wait for the event from the peer wihout
// waiting for the response of `updatePresence` here.
c1.updatePresence('name', 'c1+');
await waitFor(ClientEventType.PeersChanged, emitter1);
assert.equal(c1.getPresence().name, 'c1+');
await waitFor(ClientEventType.PeersChanged, emitter2);
c2.updatePresence('name', 'c2+');
await waitFor(ClientEventType.PeersChanged, emitter2);
assert.equal(c2.getPresence().name, 'c2+');
await waitFor(ClientEventType.PeersChanged, emitter1);
assert.deepEqual(c1.getPeers(d1.getKey()), c2.getPeers(d2.getKey()));

It means the eventual values of c1.getPeers() and c2.getPeers() become different. To be precise, c1's presence in c2.getPeers() is not updated. Although these two clients are attached to the same document, the stream initialization timing issue (mentioned above) breaks eventual consistency.

What are the relevant tickets?

Fixes #428

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #440 (8d1720a) into main (d67e898) will decrease coverage by 0.14%.
The diff coverage is 87.17%.

@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
- Coverage   89.51%   89.37%   -0.14%     
==========================================
  Files          69       69              
  Lines        5321     5289      -32     
  Branches      524      524              
==========================================
- Hits         4763     4727      -36     
- Misses        381      385       +4     
  Partials      177      177              
Impacted Files Coverage Δ
src/core/client.ts 75.86% <79.16%> (-0.59%) ⬇️
test/integration/client_test.ts 100.00% <100.00%> (ø)
test/helper/helper.ts 81.81% <0.00%> (-9.10%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hackerwins hackerwins self-requested a review January 18, 2023 08:38
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I have a few questions.

Do we need to add await this.runWatchLoop() on client.detach and client.activate as well?

src/core/client.ts Outdated Show resolved Hide resolved
@cozitive
Copy link
Contributor Author

I missed runWatchLoop() is also called in client.detach and client.activate. await keyword should also be added on them.

`Client.attach` can be terminated before getting peer presence map in
`WatchDocuments` stream initialization. It can cause presence missing when
`updatePresence` is called before the initialization.
@hackerwins
Copy link
Member

@skhugh, @7hong13, @humdrum This PR changed the response time of client.attach. It would be nice to review this PR together.

@hackerwins hackerwins self-requested a review January 20, 2023 01:01
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@hackerwins hackerwins merged commit 0134ae2 into main Jan 20, 2023
@hackerwins hackerwins deleted the missing-presence branch January 20, 2023 01:05
hackerwins pushed a commit that referenced this pull request Jan 20, 2023
`Client.attach` can be terminated before getting peer presence map in
`WatchDocuments` stream initialization. It can cause presence missing when
`updatePresence` is called before the initialization.
hunkim98 pushed a commit to hunkim98/yorkie-js-sdk that referenced this pull request Jul 12, 2023
…kie-team#440)

`Client.attach` can be terminated before getting peer presence map in
`WatchDocuments` stream initialization. It can cause presence missing when
`updatePresence` is called before the initialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Presence could be missing when calling updatePresence before receiving initialization
2 participants