websocketpeer: don't crash if Close() called multiple times

[muzzammilshahid]

Description, Motivation and Context

Refactor Close method in websocketPeer to handle duplicate close calls.

What is the current behavior?

The router crashes on duplicate close call.

What is the new behavior?

Duplicate close calls are handled.

What kind of change does this PR introduce?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• I have added tests to cover my changes.
• Overall test coverage is not decreased.
• All new and existing tests passed.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.

[om26er]

title should be "websocketpeer: don't crash if Close() called multiple times"

[om26er]

instead of starting a router and creating a websocket connection you should just mock websocket connection. You test will look something like

func TestCloseWebsocketPeer(t *testing.T) {
	logger := log.Default()
	peer := transport.NewWebsocketPeer(newMockSession(), &serialize.JSONSerializer{}, 1, logger, 0, 0)

	// Close the client connection.
	peer.Close()

	// Try closing the client connection again. It should not cause an error.
	peer.Close()
}

[muzzammilshahid]

Done

[om26er]

Looks good now. This fixes a real crash in the router.

[om26er]

@KSDaemon This fixes a panic when close is called twice on the WebSocketPeer. Would make sense to land it

[KSDaemon]

@muzzammilshahid @om26er Can you show the practical use cases when that happens? I mean when you have to call close multiple times.

[muzzammilshahid]

@KSDaemon I am facing this issue while attempting a progressive call, if the caller does not support progressive_call_invocations.
Here's the stack trace:

muzzammil@linux-machine:~/scm/xconnio/nexus/nexusd$ ./nexusd -ws 0.0.0.0:8080 -realm realm1
2024/04/05 15:32:33 Starting router v3.2.1-5-g6e73a60
2024/04/05 15:32:33 Added realm: realm1
2024/04/05 15:32:33 Listening for websocket connections on ws://0.0.0.0:8080/
2024/04/05 15:34:20 Lost 2964499830386721
panic: close of closed channel

goroutine 38 [running]:
github.com/gammazero/nexus/v3/transport.(*websocketPeer).Close(0xc000214080)
        /home/muzzammil/scm/xconnio/nexus/transport/websocketpeer.go:241 +0x4a
github.com/gammazero/nexus/v3/router.(*realm).handleSession.func1()
        /home/muzzammil/scm/xconnio/nexus/router/realm.go:410 +0x242
created by github.com/gammazero/nexus/v3/router.(*realm).handleSession
        /home/muzzammil/scm/xconnio/nexus/router/realm.go:396 +0x1ca

[KSDaemon]

@muzzammilshahid Well, okay. But this doesn't answer the question of why the second (or I would say: more than one) close() on peer is called in the first place. It looks like this PR fixes the consequences of the problem, but not the problem itself.

[muzzammilshahid]

@KSDaemon When dealer closes the session

nexus/router/realm.go

Line 410 in 6e73a60

sess.Close()

the signal is received

nexus/router/realm.go

Line 429 in 6e73a60

case msg, open = <-recv:

which returns the function

nexus/router/realm.go

Line 432 in 6e73a60

return false, false, nil

then we are trying to close a closed session here

nexus/router/realm.go

Line 410 in 6e73a60

sess.Close()

which results in panic

[om26er]

This happens when a client makes "progressive call invocations" even though it has not announced that. In that case the dealer sends the abort and closes the session

nexus/router/dealer.go

Line 660 in 6e73a60

if isInProgress && !caller.HasFeature(wamp.RoleCaller, wamp.FeatureProgCallInvocations) {

As a consequence of that the inbound message reader exits and tries to close the session

nexus/router/realm.go

Line 397 in 6e73a60

shutdown, killAll, err := r.handleInboundMessages(sess)

In any case calling WebsocketPeer.Close() should never cause a crash/panic.

[KSDaemon]

Thnx guys for the explanation! I'm afk for a weekend! I'll look deeper and probably merge in a few days!