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

Add upgrade request/response callbacks #132

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

TobiasBrandt-Talos
Copy link
Contributor

These callbacks are invoked during the handshake just before the upgrade request is sent and just after the response is received.

@TobiasBrandt-Talos TobiasBrandt-Talos self-assigned this Jun 10, 2024
These callbacks are invoked during the handshake just before the
upgrade request is sent and just after the response is received.
@TobiasBrandt-Talos TobiasBrandt-Talos force-pushed the tobiasbrandt/add-upgrade-request-response-callbacks branch from 305bf9d to f3682e8 Compare June 10, 2024 07:52
@@ -820,6 +830,10 @@ func (s *WebsocketStream) upgrade(
}
s.hb = s.hb[:0]

if s.upResCb != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be called after we assert that the handshake response is valid below? Or is there a use-case to snoop into a potentially invalid response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter. The main motivation for this is to be able to log the request/response so we can debug failed handshakes.

// just before the upgrade request is sent.
//
// The caller must not perform any operations on the stream in the provided callback.
SetUpgradeRequestCallback(callback UpgradeRequestCallback)
Copy link
Collaborator

@sergiu128 sergiu128 Jun 10, 2024

Choose a reason for hiding this comment

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

I'm guessing this is here in order to provide the http.Request to the caller for potential modification... which for the handshake means adding more headers. If so, we already have this mechanism of adding extra headers to the handshake, see here (this is used in bullish iirc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for logging the request (at least for my use case).

Copy link
Collaborator

@sergiu128 sergiu128 left a comment

Choose a reason for hiding this comment

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

Don't think the request callback is needed. The response one is ok, pending the question whether it should be invoked on a potentially wrong handshake response as well

@ethanf ethanf merged commit 0fd2669 into master Jun 18, 2024
4 checks passed
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.

4 participants