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

[Access] Configure ws connection in ws controller #6750

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Nov 21, 2024

Closes: #6638

Context

This PR implements the initial WebSocket connection configuration for the new WebSocket controller. The changes focus on setting up timeouts and ensuring the connection remains responsive, preventing the server from blocking indefinitely while waiting for the client to send or receive messages:

  • configureConnection:
    Sets an initial read deadline to prevent waiting indefinitely for a pong message.
    Establishes a Pong handler that resets the read deadline each time a pong is received, keeping the connection alive.

  • Write Deadline Timeout:
    Adds SetWriteDeadline to ensure write operations time out if not completed within writeWait, preventing resource exhaustion from slow or unresponsive clients.

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 41.22%. Comparing base (8a3055c) to head (04e392c).

Files with missing lines Patch % Lines
engine/access/rest/websockets/controller.go 46.42% 11 Missing and 4 partials ⚠️
...access/rest/websockets/legacy/websocket_handler.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6750      +/-   ##
==========================================
- Coverage   41.26%   41.22%   -0.04%     
==========================================
  Files        2061     2061              
  Lines      182702   182728      +26     
==========================================
- Hits        75384    75333      -51     
- Misses     101010   101077      +67     
- Partials     6308     6318      +10     
Flag Coverage Δ
unittests 41.22% <50.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review November 22, 2024 11:33
// message is received from the client, allowing the server to continue waiting
// for further pong messages within the new deadline.
func (c *Controller) configureConnection() error {
// Set the initial read deadline for the first pong message
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also set a close handler here?

Copy link
Contributor Author

@UlyanaAndrukhiv UlyanaAndrukhiv Nov 25, 2024

Choose a reason for hiding this comment

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

I guess that we should think aboutclose handler as part of our error handling subtask. The default close handler sends a close message back to the peer.

func (c *Conn) SetCloseHandler(h func(code int, text string) error) {
	if h == nil {
		h = func(code int, text string) error {
			message := FormatCloseMessage(code, "")
			c.WriteControl(CloseMessage, message, time.Now().Add(writeWait))
			return nil
		}
	}
	c.handleClose = h
}

https://github.com/gorilla/websocket/blob/5e002381133d322c5f1305d171f3bdd07decf229/conn.go#L1146-L1156

Copy link
Contributor

Choose a reason for hiding this comment

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

Let`s put this in #6642 just not to forget it.

Copy link
Contributor

@Guitarheroua Guitarheroua 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 to me!

@illia-malachyn
Copy link
Contributor

LGTM! Let's add tests for this in this PR as they're related #6757

@UlyanaAndrukhiv
Copy link
Contributor Author

This changes is included into PR #6757

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.

[Access] Configure ws connection in ws controller
4 participants