Skip to content

Add concurrent websocket session test #887

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.

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

Open
wants to merge 1 commit into
base: test-comm
Choose a base branch
from

Conversation

mbrandenburger
Copy link
Member

Websocket connects may be used by multiple goroutines concurrently. This commit adds a test to stress the session handling, open, send, receive, and eventual close.

The test added here triggers the issue reported in #884.

Websocket connects may be used by multiple goroutines concurrently.
This commit adds a test to stress the session handling, open, send,
receive, and eventual close.

Signed-off-by: Marcus Brandenburger <[email protected]>
@mbrandenburger mbrandenburger added the testing All related to testing label Apr 10, 2025
@adecaro
Copy link
Contributor

adecaro commented Apr 11, 2025

@mbrandenburger , please, can you fill also the PR's fields Projects and Development? Thanks.

}

func shutdown(nodes ...Node) {
// TODO: how to check that the comm service is actually stopped?
Copy link
Contributor

Choose a reason for hiding this comment

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

Stop normally waits. What is it that's failing? Maybe the connections that are not done sending?

Copy link
Member Author

Choose a reason for hiding this comment

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

See bug description in #884

}).Should(Succeed())

// close
aliceSession.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

While writing this test, did you notice the logs were giving confusing messages? Or that we didn't shut down gracefully?

Maybe we should think of more test cases. For instance if we close the channel instead of sending back pong, what do we expect to receive on the other end?

Copy link
Member Author

Choose a reason for hiding this comment

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

While writing this test, did you notice the logs were giving confusing messages? Or that we didn't shut down gracefully?

Oh the test log is wilde and outputs many warnings. Example:

2025-04-11 13:03:03.957 CEST 9c66 WARN [view-sdk.services.comm.rest.host] readMessages -> Context for stream [5E1V9rQHbsYVziNe7bHPJktJjyQBQTjYYhucotHcmEr.127.0.0.1:57126.BnsK7rcN3qKRV8y7LYUyWE0jEbfTsauQ.] closed by us. Error: %!w(*errors.errorString=&{context canceled})

Copy link
Contributor

@alexandrosfilios alexandrosfilios left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for writing the test. I think we should test this component more thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing All related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants