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.

Sign up for GitHub

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

Add doc.subscribe('status', callback) function #828

Merged
merged 7 commits into from
Jul 1, 2024
Merged

Add doc.subscribe('status', callback) function #828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f28885a
Add subscription function `doc.subscribe('status', callback)`
chacha912 May 29, 2024
c06ebfb
Merge branch 'main' of https://github.com/yorkie-team/yorkie-js-sdk i…
chacha912 Jun 14, 2024
312bdce
Trigger detached event on client.deactivate()
chacha912 Jun 14, 2024
d9bfc4c
Update test code
chacha912 Jun 14, 2024
d0863a3
Replace reattach test code
chacha912 Jul 1, 2024
3c3565c
Merge branch 'main' of https://github.com/yorkie-team/yorkie-js-sdk i…
chacha912 Jul 1, 2024
b9d9d5e
Fix lint
chacha912 Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/client/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,6 @@ export class Client {
return Promise.resolve();
}

for (const [key] of this.attachmentMap) {
this.detachInternal(key);
}

return this.rpcClient
.deactivateClient(
{
Expand All @@ -248,6 +244,11 @@ export class Client {
{ headers: { 'x-shard-key': this.apiKey } },
)
.then(() => {
for (const [key, attachment] of this.attachmentMap) {
attachment.doc.applyStatus(DocumentStatus.Detached);
this.detachInternal(key);
}
chacha912 marked this conversation as resolved.
Show resolved Hide resolved

this.status = ClientStatus.Deactivated;

logger.info(`[DC] c"${this.getKey()}" deactivated`);
Expand Down
26 changes: 26 additions & 0 deletions src/document/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ type DocEventCallbackMap<P extends Indexable> = {
'my-presence': NextFn<InitializedEvent<P> | PresenceChangedEvent<P>>;
others: NextFn<WatchedEvent<P> | UnwatchedEvent<P> | PresenceChangedEvent<P>>;
connection: NextFn<ConnectionChangedEvent>;
status: NextFn<StatusChangedEvent>;
chacha912 marked this conversation as resolved.
Show resolved Hide resolved
sync: NextFn<SyncStatusChangedEvent>;
all: NextFn<TransactionEvent<P>>;
};
Expand Down Expand Up @@ -782,6 +783,16 @@ export class Document<T, P extends Indexable = Indexable> {
error?: ErrorFn,
complete?: CompleteFn,
): Unsubscribe;
/**
* `subscribe` registers a callback to subscribe to events on the document.
* The callback will be called when the document status changes.
*/
public subscribe(
type: 'status',
next: DocEventCallbackMap<P>['status'],
error?: ErrorFn,
complete?: CompleteFn,
): Unsubscribe;
chacha912 marked this conversation as resolved.
Show resolved Hide resolved
/**
* `subscribe` registers a callback to subscribe to events on the document.
* The callback will be called when the document is synced with the server.
Expand Down Expand Up @@ -922,6 +933,21 @@ export class Document<T, P extends Indexable = Indexable> {
arg4,
);
}
if (arg1 === 'status') {
const callback = arg2 as DocEventCallbackMap<P>['status'];
return this.eventStream.subscribe(
(event) => {
for (const docEvent of event) {
if (docEvent.type !== DocEventType.StatusChanged) {
continue;
}
callback(docEvent);
}
},
arg3,
arg4,
);
}
if (arg1 === 'sync') {
const callback = arg2 as DocEventCallbackMap<P>['sync'];
return this.eventStream.subscribe(
Expand Down
2 changes: 1 addition & 1 deletion test/helper/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class EventCollector<E = string> {
resolve();
} else {
reject(
new Error(`event is not equal -
new Error(`event is not equal ${count}-
expected: ${JSON.stringify(event)},
actual: ${JSON.stringify(this.events[count - 1])}`),
);
Expand Down
158 changes: 158 additions & 0 deletions test/integration/document_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element';
import {
DocumentStatus,
DocEventType,
StatusChangedEvent,
chacha912 marked this conversation as resolved.
Show resolved Hide resolved
} from '@yorkie-js-sdk/src/document/document';
import { OperationInfo } from '@yorkie-js-sdk/src/document/operation/operation';
import { YorkieError } from '@yorkie-js-sdk/src/util/error';
Expand Down Expand Up @@ -761,6 +762,163 @@ describe('Document', function () {
await c1.deactivate();
});

it('Can subscribe to events related to document status changes', async function ({
task,
}) {
const c1 = new yorkie.Client(testRPCAddr);
const c2 = new yorkie.Client(testRPCAddr);
await c1.activate();
await c2.activate();
const c1ID = c1.getID()!;
let c2ID = c2.getID()!;

const docKey = toDocKey(`${task.name}-${new Date().getTime()}`);
const d1 = new yorkie.Document(docKey);
const d2 = new yorkie.Document(docKey);
const eventCollectorD1 = new EventCollector<StatusChangedEvent['value']>();
const eventCollectorD2 = new EventCollector<StatusChangedEvent['value']>();
const unsub1 = d1.subscribe('status', (event) => {
eventCollectorD1.add(event.value);
});
const unsub2 = d2.subscribe('status', (event) => {
eventCollectorD2.add(event.value);
});

// 1. When the client attaches a document, it receives an attached event.
await c1.attach(d1);
await c2.attach(d2);

await eventCollectorD1.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c1ID,
});
await eventCollectorD2.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c2ID,
});

// 2. When c1 detaches a document, it receives a detached event.
await c1.detach(d1);
await eventCollectorD1.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Detached,
});

// 3. When c2 deactivates, it should also receive a detached event.
await c2.deactivate();
await eventCollectorD2.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Detached,
});

// 4. When other document is attached, it receives an attached event.
const docKey2 = toDocKey(`${task.name}2-${new Date().getTime()}`);
const d3 = new yorkie.Document(docKey2);
const d4 = new yorkie.Document(docKey2);
const eventCollectorD3 = new EventCollector<StatusChangedEvent['value']>();
const eventCollectorD4 = new EventCollector<StatusChangedEvent['value']>();
const unsub3 = d3.subscribe('status', (event) => {
eventCollectorD3.add(event.value);
});
const unsub4 = d4.subscribe('status', (event) => {
eventCollectorD4.add(event.value);
});
await c1.attach(d3, { syncMode: SyncMode.Manual });
await eventCollectorD3.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c1ID,
});

await c2.activate();
c2ID = c2.getID()!;
await c2.attach(d4, { syncMode: SyncMode.Manual });
await eventCollectorD4.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c2ID,
});

// 5. When c1 removes a document, it receives a removed event.
await c1.remove(d3);
await eventCollectorD3.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Removed,
});

// 6. When c2 syncs, it should also receive a removed event.
await c2.sync();
await eventCollectorD4.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Removed,
});

// 7. If the document is in the removed state, a detached event should not occur when deactivating.
const eventCount3 = eventCollectorD3.getLength();
const eventCount4 = eventCollectorD4.getLength();
await c1.deactivate();
await c2.deactivate();
assert.equal(eventCollectorD3.getLength(), eventCount3);
assert.equal(eventCollectorD4.getLength(), eventCount4);

unsub1();
unsub2();
unsub3();
unsub4();
});

it('Should properly handle removed document when deactivating', async function ({
Copy link

Choose a reason for hiding this comment

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

  1. Would you please explain the meaning of “properly handling”?
  2. And it seems that part of validation is written in duplicate with the TC above. is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Would you please explain the meaning of “properly handling”?

It means that when another peer deactivates a deleted document, the status events occur in the order verified in the test.

  1. And it seems that part of validation is written in duplicate with the TC above. is it intended?

The event verification is duplicated when first attaching and removing to show which events occur in order. Since the important part is number 3, it might be sufficient to verify only that part.

Copy link

@maruldy maruldy Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your reply. I want some more conversation for clearly understanding.

  1. is peer means client object(e.g. c1 or c2)?

  2. According to your explanation, this tc doesn't need verification with two clients and documents. If this tc's purpose is verification with c2.deactivate(), i looks like only needed for c2 and d2. So I think it's enough with just one client and document. Would you please explain what is the point to use c1 and d1?

Copy link
Contributor Author

@chacha912 chacha912 Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your question and for seeking clarification. Let me provide more details:

  • The term peer is used to refer to a client other than oneself.
  • We need c1 and c2 because this test is about a scenario where I deactivate, but another person has removed the document.

When a client (c1) removes a document, the document's state becomes "removed," and the connection to the server is terminated.

Now, we need to verify how another client (c2) should handle receiving the Change that the document has been removed. This can be examined in two scenarios:

  1. When c2 sends a sync request (note that detach behaves similarly as it also receives the change that the document was removed):

    • This is addressed in the test above: // 6. When c2 syncs, it should also receive a removed event.

  2. When c2 deactivates without syncing (or detaching):

    • This is covered in the current test: // 3. When c2 deactivates, it should also receive a removed event.
    • For now, in the case of only deactivating, since c2 doesn't receive the Change that the document was removed, its state changes to "detached."

Copy link

Choose a reason for hiding this comment

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

Thanks for your very helpful comment.
I can almost understand after this comment why this tc was written.

I think this is the point that confused me, I didn't understand why step 2 is needed. It seems like step 2 is created to verifying for removed, but with the current specification, step 3 is checking for detached, and the test succeeds without step 2. (I tested this on my android sdk's test code)

This tc seems to be that contains part of the future spec(number2) and part of the current spec(number3) at the same time, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maruldy I'm glad my comment was helpful.

Step 2(//2. When c1 removes a document, it receives a removed event.) is a separate verification from step 3. It's not a verification of the future spec, but rather a test to verify that d1 receives a "removed" event when c1 removes the document.

In the case of the future spec, as noted, after c2 deactivates in step 3, d2 should receive a "removed" status event instead of "detached".

From the previous answer,

When a client (c1) removes a document, the document's state becomes "removed,".

This is what step 2 verifies. (d1 event)

When c2 deactivates without syncing (or detaching):

This is what step 3 verifies. (d2 event)

Since our focus is on d2, it might be sufficient to verify events only for d2 without verifying events for d1.

Copy link

Choose a reason for hiding this comment

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

@chacha912
Thanks for your reply.
Now I understand why this TC is contains step 2 and 3.

It's to validate that after calling c1.remove(), the detached event is still being received, and if the spec changes in the future to allow remove to be received, you'll want to modify this TC again then, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maruldy Yes, this test (step 3) checks if a 'detached' event occurs when c2 deactivates. And a comment is added to indicate that we might need to modify this test later if we change how things work.

Copy link

Choose a reason for hiding this comment

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

Thanks for long conversation. :) I'll replicate this tc to android sdk. 👍

task,
}) {
const c1 = new yorkie.Client(testRPCAddr);
const c2 = new yorkie.Client(testRPCAddr);
await c1.activate();
await c2.activate();
const c1ID = c1.getID()!;
const c2ID = c2.getID()!;

const docKey = toDocKey(`${task.name}-${new Date().getTime()}`);
const d1 = new yorkie.Document(docKey);
const d2 = new yorkie.Document(docKey);
const eventCollectorD1 = new EventCollector<StatusChangedEvent['value']>();
const eventCollectorD2 = new EventCollector<StatusChangedEvent['value']>();
const unsub1 = d1.subscribe('status', (event) => {
eventCollectorD1.add(event.value);
});
const unsub2 = d2.subscribe('status', (event) => {
eventCollectorD2.add(event.value);
});

// 1. When the client attaches a document, it receives an attached event.
await c1.attach(d1, {
syncMode: SyncMode.Manual,
});
await c2.attach(d2, {
syncMode: SyncMode.Manual,
});

await eventCollectorD1.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c1ID,
});
await eventCollectorD2.waitAndVerifyNthEvent(1, {
status: DocumentStatus.Attached,
actorID: c2ID,
});

// 2. When c1 removes a document, it receives a removed event.
await c1.remove(d1);
await eventCollectorD1.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Removed,
});

// 3. When c2 deactivates, it should also receive a removed event.
await c2.deactivate();
// NOTE(chacha912): For now, document status changes to `Detached` when deactivating.
// This behavior may change in the future.
Copy link

@maruldy maruldy Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@chacha912
Would you explain your plan about this comment?
Until 0.4.25-rc of Yorkie Android sdk, it doesn't update status to detached when deactivating.
I found it with yorkie-js-sdk's test code, and I'll add a commit for fix it. (But PR is not yet. I'll make it after apply 0.4.27~28.)
I made a consensus about this with @7hong13

But, I found this comment now, I want to check this how it will be changed future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maruldy At that time, there was a possibility of reattaching a document after detaching it, and it hadn't been decided whether deactivate should perform detach. So I left a NOTE.
If it's later decided that deactivate should include the same process of detaching the document, I considered that when c2 deactivates, it would receive the change of c1 removing the document, resulting in the final document status changing to removed.
Currently, when deactivating, only deactivateInternal is performed internally, changing all document statuses to detached, so the final document status changes to detached.

Copy link

Choose a reason for hiding this comment

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

I think this thread is resolved via below conversation. Thanks.

await eventCollectorD2.waitAndVerifyNthEvent(2, {
status: DocumentStatus.Detached,
});

await c1.deactivate();
unsub1();
unsub2();
});

describe('Undo/Redo', function () {
it('Can canUndo/canRedo work properly', async function ({ task }) {
type TestDoc = { counter: Counter };
Expand Down
Loading