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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 9, 2024
Merged

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

merged 3 commits into from
Apr 9, 2024

Conversation

muzzammilshahid
Copy link
Contributor

@muzzammilshahid muzzammilshahid commented Apr 2, 2024 •
edited
Loading

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.

@muzzammilshahid muzzammilshahid force-pushed the fix-close-websocket-peer branch from 6c729ef to 2798a81 Compare April 2, 2024 10:29
@muzzammilshahid muzzammilshahid force-pushed the fix-close-websocket-peer branch from 9609aab to d126d1c Compare April 2, 2024 11:36
@om26er
Copy link
Contributor

om26er commented Apr 2, 2024

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

@muzzammilshahid muzzammilshahid changed the title Fix: Refactor Close method in websocketPeer to handle duplicate close calls websocketpeer: don't crash if Close() called multiple times Apr 2, 2024
om26er
om26er reviewed Apr 3, 2024
View reviewed changes
transport/websocketpeer_test.go
"github.com/gammazero/nexus/v3/wamp"
)

func TestCloseWebsocketPeer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

om26er
om26er approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@om26er om26er left a comment

Choose a reason for hiding this comment

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

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

@om26er
Copy link
Contributor

om26er commented Apr 5, 2024

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

@KSDaemon
Copy link
Collaborator

KSDaemon commented Apr 5, 2024

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

@muzzammilshahid
Copy link
Contributor Author

@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
Copy link
Collaborator

KSDaemon commented Apr 5, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

om26er commented Apr 5, 2024

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
Copy link
Collaborator

KSDaemon commented Apr 6, 2024

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

@KSDaemon KSDaemon merged commit 588cb0f into gammazero:v3 Apr 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@om26er om26er om26er approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@muzzammilshahid @om26er @KSDaemon