Gracefully shutdown of the websocket client #213

haoqixu · 2023-11-21T07:45:15Z

Resolves #163

codecov · 2023-11-21T07:46:54Z

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.78%. Comparing base (7e92da0) to head (0c4dab0).

Files	Patch %	Lines
client/wsclient.go	75.86%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
- Coverage   73.80%   73.78%   -0.03%     
==========================================
  Files          25       25              
  Lines        1550     1579      +29     
==========================================
+ Hits         1144     1165      +21     
- Misses        296      301       +5     
- Partials      110      113       +3

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

haoqixu · 2023-11-29T08:09:08Z

Could you please take a look @evan-bradley @srikanthccv @tigrannajaryan

haoqixu · 2023-11-30T06:38:03Z

I have watched the meeting record and I think I can add some explanation about this PR.

The solution here is making wsclient.runOneCycle responsible for the entire lifecyle of the connection and controlling the running of sender and receiver. It also move the receiver to run in a separate goroutine. wsclient.runOneCycle will ensure that both sender and receiver are stopped and the connection is closed before it returns. So we don't need to care about the connection outside wsclient.runOneCycle.

When the client is shutting down, the sender goroutine will be cancelled by the ctx, send the close message to server and return the error to wsclient.runOneCycle via sender.Err(). wsclient.runOneCycle will wait for the close message from server until timeout and ensure the connection is closed.

If the server wants to close the connection, it should send the close message to initiate the close handshake. When a close message is received, the connection read methods will handle it by calling the close handler (the default handler will send a close message back to the peer. I think this is what we want.) and return a CloseError. The receiver loop will stop on any error. wsclient.runOneCycle will stop the sender and close the connection when receiver is stopped.

srikanthccv · 2023-12-17T18:58:59Z

client/wsclient.go

+		case <-r.IsStopped():
+			c.common.Logger.Debugf("shutdown handshake complete.")


Consider adding websocket.IsCloseError(r.Err(), websocket.CloseNormalClosure) as this may not always be handshake complete. Less likely but there is a chance b/w some other error could occur before the close message is received from the server.

I think the receiver loop has handled it and printed a log for it. However, the log message here is not very accurate. The "complete" here actually means that the receiver loop has also exited. 🤔

srikanthccv · 2023-12-17T19:13:21Z

client/wsclient.go

+	select {
+	case <-c.sender.IsStopped():
+		// sender will send close message to initiate the close handshake
+		if err := c.sender.Err(); err != nil && err != websocket.ErrCloseSent {


Is this err != websocket.ErrCloseSent to handle the case when the server initiates the close?

Yes. The sender and receiver run concurrently. There is a possibility that the server and client initiate the closing handshake at the same time. In this case, the close message may be sent by CloseHandler, which is called in the receiver.

Can we change sender.Err() to return nil if the error was websocket.ErrCloseSent? Let's consider it an internal implementation detail of the sender and avoid leaking it here. That we only need to check err for nil here. Sender seeing a websocket.ErrCloseSent response is a success, we don't need to consider it an error.

srikanthccv · 2023-12-17T19:19:24Z

client/internal/wssender.go

@@ -56,13 +66,22 @@ out:
 			s.sendNextMessage()

 		case <-ctx.Done():
+			s.err = s.sendCloseMessage()


This means stopping the sender also closes the connection which might be unexpected because run should simply run until the context is cancelled but it also closes the connection. I think sending Close message should be part of client Stop.

This is by design. Stopping the sender would always try to send the close message for the close handshake and runOneCycle is responsible for the subsequent steps of connection closing. I updated the comment of the runOneCycle().

srikanthccv

Overall LGTM, One main difference from this PR #205 is that the sender initiates the close handshake which makes the implementation a little simpler but also the connection management is now done b/w wsclient and wssender. @evan-bradley what do you think?

client/wsclient_test.go

evan-bradley

Thanks for doing this, overall it looks good to me. I think this approach is an improvement over #205. There may additional improvements we can make in the future, but for now this will resolve the errors we are seeing and will cleanly close the connection.

client/wsclient.go

client/internal/wsreceiver.go

client/internal/wssender.go

tigrannajaryan · 2024-01-16T15:03:52Z

client/wsclient.go

+	select {
+	case <-c.sender.IsStopped():
+		// sender will send close message to initiate the close handshake
+		if err := c.sender.Err(); err != nil && err != websocket.ErrCloseSent {


Can we change sender.Err() to return nil if the error was websocket.ErrCloseSent? Let's consider it an internal implementation detail of the sender and avoid leaking it here. That we only need to check err for nil here. Sender seeing a websocket.ErrCloseSent response is a success, we don't need to consider it an error.

client/wsclient.go

tigrannajaryan

Thanks for your patience @haoqixu

Please bear with me while I am trying to understand all the concurrent flows.

client/internal/wssender.go

client/wsclient.go

client/internal/wsreceiver.go

client/internal/wssender.go

client/wsclient.go

tigrannajaryan · 2024-01-22T15:53:14Z

client/wsclient.go

+		// sender will send close message to initiate the close handshake
+		if err := c.sender.Err(); err != nil {
+			c.common.Logger.Debugf("Error stopping the sender: %v", err)
+			break


Is there a reason we break here? If the sender stopping failed shouldn't we still try to stop the receiver and close the connection?

I think this break was introduced during the updates of the PR. The initial code does not wait for the receiver to stop, leaving the closing of the connection to the defer. I update the branch to close the conneciton explicitly and wait for the receiver to stop before break.

tigrannajaryan · 2024-01-24T23:37:35Z

Thanks for updates @haoqixu
The PR mostly looks good to me, the only remaining question is https://github.com/open-telemetry/opamp-go/pull/213/files#r1462064174

haoqixu · 2024-01-25T03:12:39Z

The PR mostly looks good to me, the only remaining question is https://github.com/open-telemetry/opamp-go/pull/213/files#r1462064174

I think this break was introduced during the updates of the PR. The initial code does not wait for the receiver to stop, leaving the closing of the connection to the defer. I update the branch to close the conneciton explicitly and wait for the receiver to stop before break.

I have updated the PR with 5fa47a3. Another option is to remove the break directly, but this will cause the sender to unnecessarily wait until timeout.

tigrannajaryan · 2024-02-01T21:18:28Z

@haoqixu this looks good to me. Can you please resolve the conflicts?

haoqixu · 2024-02-06T11:11:47Z

By design, we need to canceling the context and closing the connection to stop the ReceiverLoop. And the gracefully shutdown flow of wsclient will do this for us. Considering ReceiverLoop as an internal implementation detail of the wsclient, we could just gracefully shut down the wsclient.

gorilla/websocket doesn't provide a method to unblock the reading of connection and we always need to close the connection to unblock the reading.

#240 puts the reading of connections into a separate goroutine, which is unnecessary and may cause goroutine leaks. And the update here actually reverts #240

haoqixu · 2024-02-06T11:12:17Z

please take a look. @tigrannajaryan

tigrannajaryan · 2024-02-06T15:46:30Z

By design, we need to canceling the context and closing the connection to stop the ReceiverLoop. And the gracefully shutdown flow of wsclient will do this for us. Considering ReceiverLoop as an internal implementation detail of the wsclient, we could just gracefully shut down the wsclient.

gorilla/websocket doesn't provide a method to unblock the reading of connection and we always need to close the connection to unblock the reading.

#240 puts the reading of connections into a separate goroutine, which is unnecessary and may cause goroutine leaks. And the update here actually reverts #240

@srikanthccv please take another look at this PR since it reverts your earlier PR #240.

srikanthccv · 2024-02-06T17:44:24Z

We don't make it clear anywhere that ReceiverLoop is an internal implementation detail. By making both connection closing and context cancellation (I don't see much use of received parent context in this case) mandatory for exiting the receiver loop we may prematurely close the connection. There would be leaks if wsReceiver were to work in isolation but given we will close the connection followed by the receiver loop exit, there shouldn't be goroutine leaks?

We will need to send a final message(s) such as agent disconnect (not implemented yet) before stopping the client. I'd prefer to see wsclient have final say in when to close for such kind of reasons. I want to hear your thoughts on this.

srikanthccv · 2024-02-06T18:11:56Z

I recall we had discussed this already once. The implementation can always evolve to satisfy the needs but We should explicitly call out what we consider as internal implementation details.

Co-authored-by: Evan Bradley <[email protected]>

Co-authored-by: Srikanth Chekuri <[email protected]>

Co-authored-by: Tigran Najaryan <[email protected]>

Here is test run that shows it doesn't stop without fix https://github.com/open-telemetry/opamp-go/actions/runs/7512286887/job/20452817400 Fixes open-telemetry#239

haoqixu · 2024-03-07T07:02:44Z

I have updated the code. Please take another look. @srikanthccv @tigrannajaryan

srikanthccv

Sorry for the delay, LGTM.

jaronoff97 · 2024-03-26T21:23:18Z

@tigrannajaryan are we good to merge this? I just ran into an instance of this in the opampextension

tigrannajaryan · 2024-03-26T22:03:58Z

Thank you @haoqixu

haoqixu force-pushed the f-163-wcclient-graceful-shutdown branch from 46b92c2 to b5666be Compare November 21, 2023 07:46

haoqixu marked this pull request as ready for review November 21, 2023 15:30

haoqixu requested a review from a team November 21, 2023 15:30

srikanthccv reviewed Dec 17, 2023

View reviewed changes

srikanthccv reviewed Dec 19, 2023

View reviewed changes

client/wsclient_test.go Outdated Show resolved Hide resolved

evan-bradley reviewed Jan 9, 2024

View reviewed changes

client/wsclient.go Outdated Show resolved Hide resolved

client/wsclient.go Show resolved Hide resolved

tigrannajaryan changed the title ~~graceful shutdown of the websocket client~~ Gracefully shutdown of the websocket client Jan 9, 2024

haoqixu requested review from evan-bradley and srikanthccv January 16, 2024 07:00

srikanthccv approved these changes Jan 16, 2024

View reviewed changes

evan-bradley approved these changes Jan 16, 2024

View reviewed changes

tigrannajaryan reviewed Jan 16, 2024

View reviewed changes

haoqixu requested a review from tigrannajaryan January 16, 2024 18:12

haoqixu mentioned this pull request Jan 18, 2024

[cmd/opampsupervisor] Handle OpAMP connection settings open-telemetry/opentelemetry-collector-contrib#30237

Merged

tigrannajaryan reviewed Jan 22, 2024

View reviewed changes

srikanthccv mentioned this pull request Jan 22, 2024

ReceiverLoop should stop on cancelling context #240

Merged

haoqixu force-pushed the f-163-wcclient-graceful-shutdown branch from 165735a to fcbb3c7 Compare February 6, 2024 10:42

tigrannajaryan requested a review from srikanthccv February 6, 2024 15:45

evan-bradley mentioned this pull request Feb 8, 2024

[cmd/opampsupervisor] Leaking goroutine detected in e2e tests open-telemetry/opentelemetry-collector-contrib#30931

Closed

haoqixu and others added 20 commits March 7, 2024 11:54

graceful shutdown of the websocket client

b1d96eb

update the comments

b40a598

remove unsed code

43db955

fix: wait until both sender and receiver are stopped

90b9944

add testcase from open-telemetry#205

8dde9d9

Update client/wsclient.go

598cf25

Co-authored-by: Evan Bradley <[email protected]>

Update client/wsclient_test.go

bbe2d3f

Co-authored-by: Srikanth Chekuri <[email protected]>

Update client/wsclient.go

4c03472

Co-authored-by: Tigran Najaryan <[email protected]>

Update client/wsclient.go

84113b9

Co-authored-by: Tigran Najaryan <[email protected]>

Update client/wsclient.go

35ffed8

Co-authored-by: Tigran Najaryan <[email protected]>

Apply suggestions from code review

c6851ed

Co-authored-by: Tigran Najaryan <[email protected]>

docs: add doc comments

35c8b8d

handle websocket.ErrCloseSent inside sender

998431d

fix typo

25644c2

Apply suggestions from code review

fb226bb

Co-authored-by: Tigran Najaryan <[email protected]>

close connections explicitly

6e88dba

update logger

1446c8a

update tests

6ec846c

ReceiverLoop should stop on cancelling context (open-telemetry#240)

574baa6

Here is test run that shows it doesn't stop without fix https://github.com/open-telemetry/opamp-go/actions/runs/7512286887/job/20452817400 Fixes open-telemetry#239

fix: close stopped channel

d5c6e3a

haoqixu force-pushed the f-163-wcclient-graceful-shutdown branch from 316a4d4 to d5c6e3a Compare March 7, 2024 03:56

haoqixu marked this pull request as draft March 7, 2024 04:09

haoqixu added 2 commits March 7, 2024 14:23

test: ensure the wsclient waits for a close message from server

eb60625

fix: wsclient should wait for a close message from server

0c4dab0

haoqixu marked this pull request as ready for review March 7, 2024 07:00

srikanthccv approved these changes Mar 12, 2024

View reviewed changes

tigrannajaryan merged commit c7fc585 into open-telemetry:main Mar 26, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gracefully shutdown of the websocket client #213

Gracefully shutdown of the websocket client #213

haoqixu commented Nov 21, 2023 •

edited by tigrannajaryan

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

haoqixu commented Nov 29, 2023

haoqixu commented Nov 30, 2023

srikanthccv Dec 17, 2023

haoqixu Dec 19, 2023

srikanthccv Dec 17, 2023

haoqixu Dec 19, 2023

tigrannajaryan Jan 16, 2024

srikanthccv Dec 17, 2023

haoqixu Dec 19, 2023

srikanthccv left a comment

evan-bradley left a comment

tigrannajaryan Jan 16, 2024

tigrannajaryan left a comment •

edited

Loading

tigrannajaryan Jan 22, 2024

haoqixu Jan 24, 2024

tigrannajaryan commented Jan 24, 2024

haoqixu commented Jan 25, 2024 •

edited

Loading

tigrannajaryan commented Feb 1, 2024

haoqixu commented Feb 6, 2024

haoqixu commented Feb 6, 2024

tigrannajaryan commented Feb 6, 2024

srikanthccv commented Feb 6, 2024 •

edited

Loading

srikanthccv commented Feb 6, 2024

haoqixu commented Mar 7, 2024 •

edited

Loading

srikanthccv left a comment

jaronoff97 commented Mar 26, 2024

tigrannajaryan commented Mar 26, 2024

		case <-r.IsStopped():
		c.common.Logger.Debugf("shutdown handshake complete.")

Gracefully shutdown of the websocket client #213

Gracefully shutdown of the websocket client #213

Conversation

haoqixu commented Nov 21, 2023 • edited by tigrannajaryan Loading

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

haoqixu commented Nov 29, 2023

haoqixu commented Nov 30, 2023

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

srikanthccv left a comment

Choose a reason for hiding this comment

evan-bradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Jan 24, 2024

haoqixu commented Jan 25, 2024 • edited Loading

tigrannajaryan commented Feb 1, 2024

haoqixu commented Feb 6, 2024

haoqixu commented Feb 6, 2024

tigrannajaryan commented Feb 6, 2024

srikanthccv commented Feb 6, 2024 • edited Loading

srikanthccv commented Feb 6, 2024

haoqixu commented Mar 7, 2024 • edited Loading

srikanthccv left a comment

Choose a reason for hiding this comment

jaronoff97 commented Mar 26, 2024

tigrannajaryan commented Mar 26, 2024

haoqixu commented Nov 21, 2023 •

edited by tigrannajaryan

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

tigrannajaryan left a comment •

edited

Loading

haoqixu commented Jan 25, 2024 •

edited

Loading

srikanthccv commented Feb 6, 2024 •

edited

Loading

haoqixu commented Mar 7, 2024 •

edited

Loading