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

Fix logging of received response messages #71

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

fhs
Copy link
Contributor

@fhs fhs commented Jun 19, 2023

As documented: OnRecv causes all requests received on conn to invoke f(req, nil) and all responses to invoke f(req, resp).

Since OnRecv is called with both *Request and *Response being non-nil when we're handling a response, we need to check that *Response is non-nil before we check *Request is non-nil. This change just swaps the two cases in the switch statement to fix the issue. For consistency, I've swapped the cases for OnSend also, even when it's not needed.

As documented: OnRecv causes all requests received on conn to invoke
f(req, nil) and all responses to invoke f(req, resp).

Since OnRecv is called with both *Request and *Response being non-nil
when we're handling a response, we need to check that *Response is
non-nil before we check *Request is non-nil. This change just swaps the
two cases in the switch statement to fix the issue. For consistency,
I've swapped the cases for OnSend also, even when it's not needed.
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

thanks!

@keegancsmith keegancsmith merged commit b9c1fbd into sourcegraph:master Jul 14, 2023
@fhs fhs deleted the logmessages branch July 14, 2023 15:02
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.

2 participants