httpaf icon indicating copy to clipboard operation
httpaf copied to clipboard

Add HTTP Upgrades (based on #227)

Open yiblet opened this issue 2 years ago • 8 comments

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.

yiblet avatar Dec 22 '23 22:12 yiblet

I don’t think the other PR was reviewed

TyOverby avatar Dec 22 '23 22:12 TyOverby

LGTM regardless few details (like ;; and newlines - but I leave it to the discretion of the authors).

dinosaure avatar Dec 22 '23 22:12 dinosaure

I don’t think the other PR was reviewed

Ah I just noticed that.

yiblet avatar Dec 23 '23 00:12 yiblet

Tests hadn't run yet. I'll merge after they pass.

seliopou avatar Jan 05 '24 02:01 seliopou

Tests failed and I can't push to your remote with a fix.

seliopou avatar Jan 05 '24 02:01 seliopou

@yiblet can you allow pushing to remote for PRs?

seliopou avatar Jan 16 '24 18:01 seliopou

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.

yiblet avatar Jan 18 '24 18:01 yiblet

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

TyOverby avatar Feb 04 '24 20:02 TyOverby