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

Always send a SP after status code, even if no reason is given #36

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

showier-drastic
Copy link
Contributor

@showier-drastic showier-drastic commented Oct 9, 2024

In section 4 of RFC 9112, we can see that the status line should adhere to this format:

  status-line = HTTP-version SP status-code SP [ reason-phrase ]

Even if no reason-phrase is given, there should still be a SP (space) after status-code. edge-http does not have this second SP, if reason is empty.

Example (| marks begin and end of line):

Valid status line: |HTTP/1.1 101 |
Wrong status line (which is the current behavior): |HTTP/1.1 101|

This is causing trouble for the python websockets library, because it want two SPs in the status line (which is valid according to the spec). See code here

This can be reproduced by running the ws_server example on Linux machine, and running python3 -m websockets ws://<ip>:8881:

Failed to connect to ws://<ip>:8881: invalid HTTP status line: HTTP/1.1 101.

@ivmarkov
Copy link
Owner

ivmarkov commented Oct 9, 2024

Fair enough - thanks!

@ivmarkov ivmarkov merged commit 8c40cf9 into ivmarkov:master Oct 9, 2024
1 check passed
@showier-drastic
Copy link
Contributor Author

Actually I would suggest to remove send_status_line function, and put the logic into send_request and send_status. They are fundamentally different:

  request-line   = method SP request-target SP HTTP-version
  status-line = HTTP-version SP status-code SP [ reason-phrase ]

And, only reason-phrase here is optional. Other elements, such as method and request-target, are mandatory and cannot be empty, so I think they should not be Option<String>. if written checks can also be removed because there will always be spaces.

I can do this refactor if that sounds appropriate.

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