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.

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

Refactor Session serveMessage and mutex usage #78

Merged
merged 8 commits into from
May 28, 2024

Conversation

aruiz14
Copy link
Contributor

@aruiz14 aruiz14 commented Apr 29, 2024

Issue: 44576

Extracted a few modifications required in during the review of #74.

  • I've refactored session.serveMessage for the sake of simplicity. It now just parses the raw message and calls the appropriate method, and also moved relevant methods to an independent file.
  • Replaced the embedded sync.Mutex in Session with a RWMutex. Also, creating new methods to interact with the protected map, hence avoiding (most) of the callers to lock and unlock.
  • Abstracted wsConn into an interface, converting the existing wsConn into wsWrapper. This eases testing.

just preparing for the actual changes
@aruiz14 aruiz14 requested a review from a team as a code owner April 29, 2024 08:45
@aruiz14 aruiz14 requested review from rmweir, tomleb and a team and removed request for a team April 29, 2024 09:26
@aruiz14 aruiz14 changed the title Small refactor in Session.serveMessage Refactor Session serveMessage and mutex usage Apr 29, 2024
session.go Outdated Show resolved Hide resolved
@aruiz14 aruiz14 requested a review from rmweir May 3, 2024 08:03
rmweir
rmweir previously approved these changes May 13, 2024
Copy link
Contributor

@rmweir rmweir left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nflynt nflynt left a comment

Choose a reason for hiding this comment

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

LGTM. The connection map is much easier to follow now that the locking operations are mostly hidden away. 👍

@aruiz14 aruiz14 requested review from tomleb and removed request for tomleb May 27, 2024 15:11
@rmweir rmweir self-requested a review May 28, 2024 02:59
@nflynt nflynt merged commit c3a9b3c into rancher:master May 28, 2024
1 check passed
@aruiz14 aruiz14 deleted the refactor-session-serve branch May 28, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants