Close previous conn when switching to new conn. #351

boks1971 · 2023-11-19T03:39:48Z

This is a different attempt at
#350. Feels a bit cleaner, i. e. if the internal conn atomic.Value is replaced, the previous conn is closed before doing that.

This is a different attempt at #350. Feels a bit cleaner, i. e. if the internal `conn atomic.Value` is replaced, the previous `conn` is closed before doing that.

davidzhao · 2023-11-19T17:21:41Z

signalclient.go

-		if c.OnClose != nil {
-			c.OnClose()
-		}
+		c.conn.Store((*websocket.Conn)(nil))


we cannot store nil into an atomic.Value; it'll panic: https://pkg.go.dev/sync/atomic#Value.Store

would be good to use typed atomic.Pointer instead.

Casting nil to the concrete type does not cause a panic. Learnt that today. I can't find that post now, but here is a Go Playground example - https://go.dev/play/p/-O0qiTGJxJs>

But, I will change to look at atomic.Pointer

Right, because a reference to an interface with a type but a nil object is not nil, which makes nil checks really confusing at time. Ask me how I know...

How do you know @biglittlebigben 😊?

Panic in prod at 3am. It's always a panic in prod at 3am...

Using atomic.Pointer in this commit.

NOTE: There is comment in README.md that this module requires go 1.18+ since version 1.0. Need to update that to 1.19+ when we cut a release.

davidzhao · 2023-11-19T17:24:09Z

signalclient.go

 		res, err := c.readResponse()
 		if err != nil {
-			if !isIgnoredWebsocketError(err) && !c.isClosed.Load() {


we'll also need to make sure readResponse returns EOF if it's expected close. otherwise it'll keep spamming logs about error while reading.

Will look at this.

On second thought, this does not do anything different. A unilateral close (by calling Close() method) will close the underlying conn and read will fail with reading a closed conn. And this loop will exit/return.

Before this change, the code was only checking isClosed which is set by the unilateral Close().

The remote close will be an error returned by ReadMessage. Yes, this change will log one message more because it will log an error on an unilateral Close() too. But, nothing more.

Once an error is returned, the loop is exited.

Am I missing something?

maybe I'm reading this incorrectly.. here it would previously return io.EOF when Close has been called due to the c.isClosed.Load() check.. currently I think it'll go into errors.New("cannot read response before join") ?

It seems that we should not return an error if Close has been called.

That's correct @davidzhao . You are reading it right. But, what I was saying is that Close() in this PR sets the conn to nil (the other discussion about atomic.Value == nil, I will change that to an atomic.Pointer). So, yes readResponse will return errors.New("cannot read response before join").

But, the readLoop will exit because of that error return.

Only difference is that previously, the read loop would not have logged an error before exiting as it was checking for isClosed and with this PR, it will log an error before exiting. So, what I was mentioning was that I could not spot any runaway loops repeatedly logging an error.

Sorry, I didn't mean it would be in a loop. Just that in server environments, it would be good to avoid logging errors when the user has closed the connection.

Got it, I misunderstood. I will look at adding a change to filter out the error

Addressed in this PR - #353

biglittlebigben · 2023-11-19T18:05:48Z

This seems to fix the cloud-ingress restart issue.

davidzhao · 2023-11-20T01:59:42Z

signalclient.go

 		res, err := c.readResponse()
 		if err != nil {
-			if !isIgnoredWebsocketError(err) && !c.isClosed.Load() {


maybe I'm reading this incorrectly.. here it would previously return io.EOF when Close has been called due to the c.isClosed.Load() check.. currently I think it'll go into errors.New("cannot read response before join") ?

It seems that we should not return an error if Close has been called.

cnderrauber · 2023-11-20T02:21:13Z

signalclient.go

@@ -348,18 +350,17 @@ func (c *SignalClient) handleResponse(res *livekit.SignalResponse) {
 func (c *SignalClient) readWorker() {
 	defer func() {
 		c.isStarted.Store(false)
-		if c.OnClose != nil {
-			c.OnClose()


This means OnClose will only be called when Close is called instead of connection is interrupted, and the Engine relies this callback to handle signal disconnection and resume.

Thank you @cnderrauber . Will take a look. Was hitting a data race in the tests with the OnClose() in the defer. So, I serialised it :-( My bad. Will take a look at calling back on read loop exit.

@cnderrauber Made changes in these two commits

9a727bc - moving OnClose() callback back to defer of readWorker.

0acef17 - using a local variable to avoid race. The race reported before is something like this. The test was calling simulate scenario reconnect which called signal client close. And the read on readerCloserCh in Close() was being accessed in both Close() and Start() (where it is changed to set up for next read worker). Getting around it by using a local variable in Close() and read on that.

Please have a look when you get a chance and let me know if you have concerns.

cnderrauber

Looks good! Thanks for fixing this.

Close previous conn when switching to new conn.

04769f2

This is a different attempt at #350. Feels a bit cleaner, i. e. if the internal `conn atomic.Value` is replaced, the previous `conn` is closed before doing that.

boks1971 requested review from davidzhao, cnderrauber and biglittlebigben November 19, 2023 03:39

boks1971 added 3 commits November 19, 2023 09:28

Prevent race

dc71bfb

race

7aa190c

check nil

b2c826c

davidzhao reviewed Nov 19, 2023

View reviewed changes

davidzhao approved these changes Nov 20, 2023

View reviewed changes

cnderrauber reviewed Nov 20, 2023

View reviewed changes

boks1971 added 4 commits November 20, 2023 10:25

Use atomic.Pointer

0e1c8fe

move OnClose back to defer, need to deal with race differently

9a727bc

use a local variable

0acef17

nil check, just being paranoid

a5825d6

cnderrauber approved these changes Nov 20, 2023

View reviewed changes

boks1971 merged commit 65f60f3 into main Nov 20, 2023
2 checks passed

boks1971 deleted the raja_close2 branch November 20, 2023 08:12

boks1971 mentioned this pull request Nov 20, 2023

Close the correct connection. #350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close previous conn when switching to new conn. #351

Close previous conn when switching to new conn. #351

boks1971 commented Nov 19, 2023

davidzhao Nov 19, 2023

boks1971 Nov 19, 2023

biglittlebigben Nov 19, 2023

boks1971 Nov 19, 2023

biglittlebigben Nov 19, 2023

boks1971 Nov 20, 2023 •

edited

Loading

davidzhao Nov 19, 2023

boks1971 Nov 19, 2023

boks1971 Nov 19, 2023

davidzhao Nov 20, 2023

boks1971 Nov 20, 2023 •

edited

Loading

davidzhao Nov 20, 2023

boks1971 Nov 20, 2023

boks1971 Nov 20, 2023

biglittlebigben commented Nov 19, 2023

davidzhao Nov 20, 2023

cnderrauber Nov 20, 2023

boks1971 Nov 20, 2023

boks1971 Nov 20, 2023 •

edited

Loading

cnderrauber left a comment

Close previous conn when switching to new conn. #351

Close previous conn when switching to new conn. #351

Conversation

boks1971 commented Nov 19, 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

boks1971 Nov 20, 2023 • edited Loading

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

boks1971 Nov 20, 2023 • edited Loading

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

biglittlebigben commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boks1971 Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

cnderrauber left a comment

Choose a reason for hiding this comment

boks1971 Nov 20, 2023 •

edited

Loading

boks1971 Nov 20, 2023 •

edited

Loading

boks1971 Nov 20, 2023 •

edited

Loading