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

Use gorilla readMessage and writeMessage instead of just an io.Copy #2640

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

juliens
Copy link
Member

@juliens juliens commented Jan 2, 2018

What does this PR do?

Fix a bug in websocket
related to: containous/oxy#48

Motivation

Fixes #2629

More

Fix a bug with packet loss in websocket underlying connection. Instead of use this connection, we read messages with gorilla and write messages with gorilla

@juliens juliens added area/websocket bot/light-review decreases the number of required LGTM from 3 to 1. kind/bug/fix a bug fix status/2-needs-review labels Jan 2, 2018
@juliens juliens added this to the 1.4 milestone Jan 2, 2018
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 52f16e1 into traefik:v1.4 Jan 2, 2018
@traefiker traefiker removed bot/light-review decreases the number of required LGTM from 3 to 1. status/3-needs-merge labels Jan 2, 2018
@seriousben
Copy link

@juliens could you explain why this change was required? (Just trying to get my head around it.)

@juliens
Copy link
Member Author

juliens commented Jan 4, 2018

Some servers embed some messages in the body of the switching protocol response. Those messages are not in the underlying connection we used before this change.
But this issue is handle (internally) in the ReadMessage mechanism with a buffer reader. That's why we need to do this until we can directly use the buffer reader.

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

Successfully merging this pull request may close these issues.

4 participants