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

Config value for reading frames as passthrough #371

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Rwardc
Copy link

@Rwardc Rwardc commented Aug 3, 2023

Reasoning
I've added this new, hopefully non-invasive config value to the WebSocketConfig struct to allow for the following:

  • Preserve extended frame headers (non-zero reserved bits, à la RFC 7692 ), until such a time as these are officially supported
  • Facilitate true proxying by passing received frames as-is to upstream handlers with minimal interference
  • Allow for post-processing of websocket frames in applications where current tungstenite configuration does not allow enough customization

We are currently working on a rust-based proxy project that uses a library that in turn uses tungstenite. We were experiencing issues proxying the websocket traffic and tracked the issue down to tungstenite dropping all extended frames. I see this as a temporary issue until negotiated extensions are handled by tungstenite, or a proper API for processing raw extended frames is provided.

Copy link
Member

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to reply to this - I was waiting for CI to succeed, but for some reason, the pipeline did not run for your PR (it might be that it happened while fixes to the pipelines were in progress).

I'm generally not opposed to the concept of introducing this logic, but I'm not sure if the suggested change is mostly for 2 reasons:

  1. I feel like it's not suitable for a long-term solution. As you've already noted, this might be considered a temporary change, which makes me think if it's actually something we want to have.
  2. The state machine code gets some additional branching and complexity (honestly, it's already quite complex to the point that we would need to refactor it before doing this change), but additional handling of the read_as_frames option by checking the frame one more time, calling do_close() etc, makes it a bit harder to verify.

@Rwardc
Copy link
Author

Rwardc commented Mar 14, 2024

We ended up forking this repo to handle the issue. We are writing a proxy and needed error free tunneling of websocket frames without any intermediate processing. This config value helped to achieve this goal.

In answer to your questions:

  1. This was indeed intended as a temporary solution, one we've already added to our fork.
  2. Because of our particular use case, I didn't take the config complexity into consideration.

I would be interested in the level of effort required to get this into the mainline, especially since we don't necessarily like to keep long lived forks.

@daniel-abramov
Copy link
Member

I would be interested in the level of effort required to get this into the mainline, especially since we don't necessarily like to keep long lived forks.

Alright! So what do we do with the PR then? Would you be fine closing this PR and then opening a new one should you decide for a "clean" (long-term) implementation?

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