Add HTTP Upgrades (based on #227)
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.
I don’t think the other PR was reviewed
LGTM regardless few details (like ;; and newlines - but I leave it to the discretion of the authors).
I don’t think the other PR was reviewed
Ah I just noticed that.
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