Hide Watch method from Client.go and move into Attach method #593

karockai · 2023-07-31T13:51:58Z

What this PR does / why we need it:

make Watch method of Client to private.
make Attach method to execute the watch method atomically after attaching.
modify some related tests.

Which issue(s) this PR fixes:
Fixes #584

Special notes for your reviewer:
There is one problem in this PR. After a client watching is changed to execute continuously after attaching a document, some tests in document_test have had an issue. Please see the scenario below.

the client tries to attach 'doc1'
the client tries to detach the doc
the client tries to attach another doc

There is no problem Until step 2 but step3 has.
The err is closing transport due to: connection error: desc = "error reading from server: EOF", received prior goaway: code: NO_ERROR
and server log is 2023-07-30T23:53:36.646+0900 WARN r7 RPC : stream "/yorkie.v1.YorkieService/WatchDocument" 9.632547583s => "rpc error: code = Unknown desc = connection error: desc = \"transport is closing\""

I said above that it wouldn't be a problem in step 2, but it really isn't. Even though error checking tells there is no err after step 2, to pass tests a time buffer should be set between step 1 and step 2, and also step 2 and step 3. The time buffer should be more than 4 sec. The test couldn't be passed when there is no time buffer between step 2 and step 3,

After tried to solve this problem several days, I found that it is better to share this problem rather than try only myself.

further detail:
client side err is coming from

# Client.watch L:482
	pbResp, err := stream.Recv()
	if err != nil {
		return nil, err
	}

server side err is coming from

#yorkie_server.WatchDocument L:411
	if err := stream.Send(&api.WatchDocumentResponse{
		Body: &api.WatchDocumentResponse_Initialization_{
			Initialization: &api.WatchDocumentResponse_Initialization{
				ClientIds: pbClientIDs,
			},
		},

Does this PR introduce a user-facing change?:

- Client provides `Watch` no more. `Attach` method will execute watching continuously. so remove your `Watch` from where to use.

Additional documentation:

Checklist:

Added relevant tests or not required
Didn't break anything

CLAassistant · 2023-07-31T13:52:03Z

All committers have signed the CLA.

codecov · 2023-08-05T06:37:28Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (a8f6bc2) 50.71% compared to head (93ee546) 51.44%.

❗ Current head 93ee546 differs from pull request most recent head 25ed0ef. Consider uploading reports for the commit 25ed0ef to get more accurate results

Files	Patch %	Lines
client/client.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   50.71%   51.44%   +0.72%     
==========================================
  Files          70       66       -4     
  Lines       10213     6978    -3235     
==========================================
- Hits         5180     3590    -1590     
+ Misses       4512     2912    -1600     
+ Partials      521      476      -45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

krapie · 2023-08-08T13:37:35Z

I will also review this. It will take some time. @karockai

hackerwins · 2023-08-10T08:07:41Z

I think this task is not as simple as it might seem. To move watch method into attach, it requires careful management of the watch stream. This ensures that the stream is maintained, allowing for the attempt of watching in case of failure, along with handling the "cancel".

You can refer to Client.Attach in JS SDK:

https://github.com/yorkie-team/yorkie-js-sdk/blob/53185d5f3d69cc65bbc109729ce0a5a8a0c8ca1a/src/client/client.ts#L437-L497

And we need to list what more to consider and decide on the scope of work.
@chacha912 Do you have any comments?

krapie

There is one problem in this PR. After a client watching is changed to execute continuously after attaching a document, some tests in document_test have had an issue. Please see the scenario below.
...

This is because the watch stream created by attach(step 1) is not closed even after detach(step 2). So when the client tries to perform attach(step 3), watch stream collides with the existing watch stream (I have to investigate more about this).

Since the watch stream automatically closes after MaxConnectionAge + MaxConnectionAgeGrace which is configured in the server (for integration test, it is set to 8 seconds + 2 seconds), time buffer you have set (which is total time.Sleep(5) + time.Sleep(5)) will eventually close the stream. This is why your test is passing.

To conclude, you need to close the stream on detach. In the JS SDK, detachInternal()'s cancelWatchStream() performs this task.

krapie · 2023-09-21T03:39:57Z

@karockai It seems like there is a data race on attachment.watchStream in integration test.

Concurrent deletion of attachment.watchStream in cancelWatchStream() and read of attachment.watchStream in if attachment.watchStream == nil is causing data race.

WARNING: DATA RACE
Write at 0x00c0006a4048 by goroutine 127:
  github.com/yorkie-team/yorkie/client.(*Attachment).cancelWatchStream()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:688 +0x47c
  github.com/yorkie-team/yorkie/client.(*Client).pushPullChanges()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:617 +0x43c
  github.com/yorkie-team/yorkie/client.(*Client).Sync()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:402 +0x118
  github.com/yorkie-team/yorkie/test/integration.TestAdmin.func2()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/test/integration/admin_test.go:83 +0x504
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1629 +0x40

Previous read at 0x00c0006a4048 by goroutine 179:
  github.com/yorkie-team/yorkie/client.(*Client).watch.func3()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:520 +0x280

Goroutine 127 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1629 +0x5e4
  github.com/yorkie-team/yorkie/test/integration.TestAdmin()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/test/integration/admin_test.go:54 +0x2a4
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1629 +0x40

Goroutine 179 (running) created at:
  github.com/yorkie-team/yorkie/client.(*Client).watch()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:495 +0x3e0
  github.com/yorkie-team/yorkie/client.(*Client).Attach()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/client/client.go:327 +0x7e8
  github.com/yorkie-team/yorkie/test/integration.TestAdmin.func2()
      /Users/krapi/Developer/Opensource/Yorkie/yorkie/test/integration/admin_test.go:71 +0x384
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1629 +0x40

krapie

Thank you for your contribution.

I'm still reviewing on watch() on client.go, but I left a brief comments and questions.

krapie · 2023-09-28T17:37:55Z

client/client.go

-	}
-	if _, err := handleResponse(pbResp); err != nil {
-		return nil, err
+	rch := make(chan WatchResponse, 3)


Is there any reason for setting buffer size to 3?

krapie · 2023-09-28T17:38:57Z

pkg/document/document.go

@@ -79,7 +79,7 @@ type Document struct {
 func New(key key.Key) *Document {
 	return &Document{
 		doc:    NewInternalDocument(key),
-		events: make(chan DocEvent, 1),
+		events: make(chan DocEvent, 3),


Is there any reason for setting buffer size to 3?

Same here.

krapie · 2023-09-28T17:39:02Z

server/backend/sync/pubsub.go

@@ -36,7 +36,7 @@ func NewSubscription(subscriber *time.ActorID) *Subscription {
 	return &Subscription{
 		id:         xid.New().String(),
 		subscriber: subscriber,
-		events:     make(chan DocEvent, 1),
+		events:     make(chan DocEvent, 3),


Is there any reason for setting buffer size to 3?

Same here.

krapie · 2023-09-29T09:19:17Z

test/integration/agent_test.go

-	"github.com/yorkie-team/yorkie/test/helper"
-)
-
-func TestServer(t *testing.T) {


Is there any reason for removing this test?

krapie · 2023-09-29T09:25:41Z

test/bench/grpc_bench_test.go

@@ -287,13 +284,15 @@ func BenchmarkRPC(b *testing.B) {
 				wg.Add(2)
 				go func() {
 					defer wg.Done()
-					err := c1.Attach(ctx, doc1)
+					rch1, err := c1.Attach(ctx, doc1)


I wonder if it is necessary to receive and check rch1 channel every time we perform Attach(), even if the test does not require rch channel.

Maybe setting rch1 to _ would be better.

If you think this suggestion is reasonable, please check other test codes changes that you have made to apply this suggestion.

krapie · 2023-09-29T10:15:58Z

test/integration/document_test.go


 		// 03. abnormal behavior on removed state
 		assert.NoError(t, cli.Remove(ctx, d1))
 		assert.ErrorIs(t, cli.Remove(ctx, d1), client.ErrDocumentNotAttached)
 		assert.ErrorIs(t, cli.Sync(ctx, client.WithDocKey(d1.Key())), client.ErrDocumentNotAttached)
 		assert.ErrorIs(t, cli.Detach(ctx, d1), client.ErrDocumentNotAttached)
 	})
-
-	t.Run("removed document removal with watching test", func(t *testing.T) {


Is there any reason for removing this test?

krapie · 2023-09-29T11:02:21Z

test/integration/presence_test.go

+		defer func() {
+			assert.NoError(t, c1.Detach(ctx, d1))
+		}()
+		d2 := document.New(helper.TestDocKey(t), 2)


What does this second parameter 2 mean?

krapie · 2023-09-29T11:15:25Z

test/integration/presence_test.go

 						return
 					}
 				}
 			}
 		}()

 		// 03. Watch the second client's document.
-		expected = append(expected, watchResponsePair{


Any reason for removing expected code and changing len(responsePairs) == 3 to len(responsePairs) == 1?

krapie · 2023-09-29T11:25:50Z

pkg/document/document.go

@@ -79,7 +79,7 @@ type Document struct {
 func New(key key.Key) *Document {
 	return &Document{
 		doc:    NewInternalDocument(key),
-		events: make(chan DocEvent, 1),
+		events: make(chan DocEvent, 3),


Is there any reason for changing buffer to 3?

krapie · 2023-09-29T11:25:55Z

server/backend/sync/pubsub.go

@@ -36,7 +36,7 @@ func NewSubscription(subscriber *time.ActorID) *Subscription {
 	return &Subscription{
 		id:         xid.New().String(),
 		subscriber: subscriber,
-		events:     make(chan DocEvent, 1),
+		events:     make(chan DocEvent, 3),


Is there any reason for changing buffer to 3?

Same here.

…rface remove Client.Watch from external interface and move into Client.Attach, so watching a document will be executed continously after attaching continuously (and atomically).

krapie · 2024-02-13T12:20:27Z

@hackerwins I will look this PR again and finish this task.

hackerwins · 2024-02-14T00:31:53Z

@krapie Thanks for your interest. It would be good to recreate PR.

karockai force-pushed the hide_watch_in_client branch 6 times, most recently from 7dbefe0 to 93ee546 Compare August 5, 2023 06:30

krapie requested a review from hackerwins August 6, 2023 04:47

krapie assigned krapie and karockai and unassigned krapie Aug 6, 2023

krapie self-requested a review August 8, 2023 13:37

karockai marked this pull request as draft August 12, 2023 07:42

krapie reviewed Aug 12, 2023

View reviewed changes

hackerwins force-pushed the main branch from ac41998 to 932a9c7 Compare August 18, 2023 00:59

karockai changed the title ~~hide Watch method from Client.go and move into Attach method~~ Hide Watch method from Client.go and move into Attach method Aug 19, 2023

karockai force-pushed the hide_watch_in_client branch from 49ce076 to 93ee546 Compare September 12, 2023 15:03

hackerwins force-pushed the main branch from 7de98b5 to 940941a Compare September 13, 2023 02:56

krapie self-requested a review September 21, 2023 03:19

krapie reviewed Sep 29, 2023

View reviewed changes

hackerwins unassigned karockai Nov 1, 2023

hackerwins force-pushed the main branch 3 times, most recently from 7892142 to fdc2e1c Compare November 4, 2023 10:22

hackerwins force-pushed the main branch from c7b445a to 17d0039 Compare November 23, 2023 00:57

hackerwins force-pushed the main branch from 31c1023 to b399a68 Compare January 25, 2024 09:21

karockai added 2 commits February 13, 2024 21:16

Move Client.Watch inside Client.Attach and hide it from external inte…

840118f

…rface remove Client.Watch from external interface and move into Client.Attach, so watching a document will be executed continously after attaching continuously (and atomically).

manage watch stream of attachment in go routine

25ed0ef

krapie force-pushed the hide_watch_in_client branch from cc29fac to 25ed0ef Compare February 13, 2024 12:16

hackerwins closed this Feb 14, 2024

krapie mentioned this pull request Feb 28, 2024

Move Client.Watch inside Client.Attach #803

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide Watch method from Client.go and move into Attach method #593

Hide Watch method from Client.go and move into Attach method #593

karockai commented Jul 31, 2023

CLAassistant commented Jul 31, 2023 •

edited

Loading

codecov bot commented Aug 5, 2023 •

edited

Loading

krapie commented Aug 8, 2023

hackerwins commented Aug 10, 2023

krapie left a comment •

edited

Loading

krapie commented Sep 21, 2023 •

edited

Loading

krapie left a comment

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie Sep 29, 2023

krapie commented Feb 13, 2024

hackerwins commented Feb 14, 2024

Hide Watch method from Client.go and move into Attach method #593

Hide Watch method from Client.go and move into Attach method #593

Conversation

karockai commented Jul 31, 2023

CLAassistant commented Jul 31, 2023 • edited Loading

codecov bot commented Aug 5, 2023 • edited Loading

Codecov Report

krapie commented Aug 8, 2023

hackerwins commented Aug 10, 2023

krapie left a comment • edited Loading

Choose a reason for hiding this comment

krapie commented Sep 21, 2023 • edited Loading

krapie 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krapie commented Feb 13, 2024

hackerwins commented Feb 14, 2024

CLAassistant commented Jul 31, 2023 •

edited

Loading

codecov bot commented Aug 5, 2023 •

edited

Loading

krapie left a comment •

edited

Loading

krapie commented Sep 21, 2023 •

edited

Loading