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

Handling ControlData::Request when decapsulating responses is awkward #66

Open
tgeoghegan opened this issue May 23, 2024 · 2 comments
Open

Comments

@tgeoghegan
Copy link
Contributor

When handling the response from an OHTTP relay, we must decapsulate (decrypt) the relay's response and then decode the relay's response body into a bhttp::Message. To get the HTTP status of the encapsulated response, I then have to examine the bhttp::ControlData, which is either ControlData::Request or ::Response.

This is awkward, because if I'm decapsulating a response, why would it ever be ControlData::Request? In glean (which IIUC goes into Firefox), they handle this by using ControlData::status -> Option<StatusCode>, and wrapping the None case in a fatal error. I had to write similarly annoying code in Janus.

Here's my suggestion: we know from context that we expect a response and only a response. Additionally, we already have to handle decoding errors that might arise from Message::read_bhttp. What if instead of Message, there were explicit Request and Response types, and their read_bhttp methods would fail if the bytes couldn't be decoded as a request or response, respectively? I think this would also resolve some ambiguity around Message's informational_response field.

@tgeoghegan
Copy link
Contributor Author

I'd be willing to work on such a change if this sounds reasonable to the maintainer(s).

@martinthomson
Copy link
Owner

This seems like a reasonable thing to do, for sure. If you are willing to do some work, I'd be happy to review.

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

No branches or pull requests

2 participants