Add HTTP Upgrades (based on #227) #228
Conversation
|
I don’t think the other PR was reviewed |
|
LGTM regardless few details (like |
yiblet
changed the title
Ah I just noticed that. |
tmcgilchrist
left a comment
tmcgilchrist
left a comment
There was a problem hiding this comment.
A few small differences between the upgrade code in @anmonteiro's fork and this implementation.
| | Streaming _ -> | ||
| failwith "httpaf.Reqd.respond_with_upgrade: response already started" | ||
| | Upgrade _ | ||
| | Fixed _ -> | ||
| failwith "httpaf.Reqd.respond_with_upgrade: response already complete" |
There was a problem hiding this comment.
The logic for @anmonteiro's changes has Streaming | Upgrade grouped together while here it is Upgrade | Fixed
There was a problem hiding this comment.
I think the upgrade response is more similar to the fixed response here than the streaming response.
Streaming has a different error message since you can't assume that the response has completed in this call. Upgrade and Fixed both have concrete payloads that respond in a similar manner so I'd say they both likely to have completed the response.
| if is_active t then Reqd.flush_response_body (current_reqd_exn t); | ||
| Writer.close t.writer; | ||
| wakeup_writer t | ||
| if is_active t | ||
| then Reqd.close_request_body (current_reqd_exn t) | ||
| else wakeup_writer t |
There was a problem hiding this comment.
Is the change in ordering important here? It seems like it is doing a second if is_active t check to close the request body in a different order to previously.
|
Tests hadn't run yet. I'll merge after they pass. |
|
Tests failed and I can't push to your remote with a fix. |
|
@yiblet can you allow pushing to remote for PRs? |
|
Done! I added you as a collaborator, so I think you should be able to push fixes once you accept that. Let me know if that doesn't work though. |
|
A recent bugfix that prevents consuming data out of a connection that has already been upgraded. diff --git a/lib/server_connection.ml b/lib/server_connection.ml
--- a/lib/server_connection.ml
+++ b/lib/server_connection.ml
@@ -273,7 +273,12 @@ let rec read_with_more t bs ~off ~len mo
);
(* Keep consuming input as long as progress is made and data is
available, in case multiple requests were received at once. *)
- if consumed > 0 && consumed < len then
+ let reader_wants_more =
+ is_active t && match Reqd.output_state (current_reqd_exn t) with
+ | Upgraded -> false
+ | Waiting | Ready | Complete -> true
+ in
+ if consumed > 0 && consumed < len && reader_wants_more then
let off = off + consumed
and len = len - consumed in
consumed + read_with_more t bs ~off ~len more |
5 participants
This PR is an exact copy of #227 made by @TyOverby. I just removed the added sexplib library and sexp ppx. It was clear from that other PR that there was a lot of contention with adding that dependency but there wasn't as much concern with the other core value add: the changes to make HTTP upgrades easier.
To separate out the review of adding sexp deps from the review of adding this http upgrades, I made this PR. Ideally since PR #227 was already reviewed, it should be easier to process this PR.
If there are any other concerns with this PR I'd be happy to address them and see what I can do to push this feature along.