add Reqd.schedule_trailers (fixes #97) #141

blandinw · 2020-09-27T01:22:26Z

This PR makes sending trailing headers (aka. trailers) possible, and thus allows H2 to be used to implement gRPC.
I've been going back and forth on the best patch to write, and I believe this strikes a good balance, but I'm curious to hear your thoughts.
I had a look at the changes proposed in #72 and I believe it suffers from the sequencing issue described below (commit #1).
However, I did steal the test case from there.

Commit 1 (move Streaming flush logic out of Body):

Body.close_writer caused stream to close before we had the chance to send headers
Body.close_writer caused next flush to send a zero length DATA frame, which leads to a sequence like HEADERS, DATA, TRAILERS, DATA
imo, Body should not be concerned with HTTP details (knowing about RFC7540 and sending DATA frames)
moving the corresponding code out of Body and into Reqd feels like the right thing to do since it allows for better sequencing + simpler code (has_pending_output forwards to Faraday, no more t.write_final_data_frame and no more response_body_requires_output)
I also noticed a potential bug where I believe the zero length DATA frame is never sent if we're flow-controlled (maxxed out sent bytes) in Scheduler.write

Commit 2 (allow trailing headers, fixes #97)

straight-forward thanks to the sequencing control allowed by commit 1
we choose not to let user send trailers directly, but instead register them and later send them on Body.close to avoid user mistakes (sending trailers too early or too late)
this also allows to avoid sending a useless zero length DATA frame before the trailers

Quick note: I was confused at the beginning as to how the state machine worked, until I realized it relied on Gluten. It's not obvious at first if you're grepping around (could not find next_write_operation call sites)
You may want to have a quick blurb or schema showing the Gluten -> Server_connection -> Scheduler -> Reqd -> Writer -> Faraday chain.

anmonteiro · 2020-09-27T01:42:31Z

Thanks for sending this. I'll try to get to it this weekend.

Before I start reviewing, a potentially related question: would gRPC also need to read trailer headers, in addition to sending them (what this PR does)? There's some machinery in place for that but it's not fully wired up.

blandinw · 2020-09-27T01:53:33Z

Hi Antonio, thanks!

Clients need the ability to read the response trailers since they contain the actual status code (HTTP code should always be 200, regardless of errors), and an optional message. Clients do not need the ability to send request trailers (nor do servers need to read request trailers) as far as I know.

anmonteiro · 2020-09-27T02:33:55Z

I also noticed a potential bug where I believe the zero length DATA frame is never sent if we're flow-controlled (maxxed out sent bytes) in Scheduler.write

@blandinw I think you're right about this. If you don't mind, I'm going to push a test + a fix for that before discussing this PR further.

anmonteiro

@blandinw I like the direction of this PR and the approach you took. Thanks again for sending it over!

I only left mostly minor stylistic / documentation comments. Additionally, would you mind adding an entry to the CHANGES.md file?

Note: I force-pushed to this branch after merging #142, so please make sure to reset locally.

lib/h2.mli

lib/reqd.ml

anmonteiro · 2020-09-28T00:04:56Z

lib/reqd.ml

+let send_trailers_on_close t new_trailers =
+  let go s =
+    match s.response_state with
+    | Streaming (rsp, rsp_body, _old_trailers) ->


thoughts on failing if you try to schedule multiple times? I think this behavior should be somewhat specified, and additionally documented in the .mli

I can't think of a use case where we'd need to change the trailers after setting them, and we should notify the user somehow that something is probably wrong.

I was hoping we could somehow avoid runtime errors, but I'm not sure how to do it with the current API. Maybe something to think about if we decide to change the API one day (we could then probably enforce it at the type level).

I'll error out for now.

anmonteiro · 2020-09-28T00:07:14Z

lib/reqd.ml

+  in
+  match t.state with
+  | Idle | Active (Open (WaitingForPeer | PartialHeaders _), _) | Closed _ ->
+    assert false


I think it's possible, in certain cases, to retain a reference to the request descriptor when the stream has been closed (e.g. because of an error). The other cases seem fine to me, but the Closed case should probably be handled separately.

I fail with a useful error message in the Closed and Reserved cases.
Let me know if this is what you had in mind.

lib/reqd.ml

lib/serialize.ml

anmonteiro · 2020-09-28T00:16:00Z

lib_test/test_h2_server.ml

+      Alcotest.fail
+        "Expected state machine to issue a write operation after seeing \
+         headers."
+
  (* TODO: test for trailer headers. *)


remove TODO please :)

lib_test/test_h2_server.ml

blandinw

Addressed your comments in a new commit for easier review, please squash before merging

blandinw · 2020-09-28T12:29:19Z

lib/reqd.ml

+  in
+  match t.state with
+  | Idle | Active (Open (WaitingForPeer | PartialHeaders _), _) | Closed _ ->
+    assert false


I fail with a useful error message in the Closed and Reserved cases.
Let me know if this is what you had in mind.

blandinw · 2020-09-28T12:31:49Z

lib/reqd.ml

-            ~max_frame_size:t.max_frame_size
-            ~flags:Flags.(set_end_stream default_flags)
-            t.id
+        let frame_info flags =


Correct, I used to send an extra DATA frame, but changed my mind.

anmonteiro · 2020-09-28T20:08:14Z

lib/reqd.ml

+            stream.encoder
+            frame_info
+            trailers;
+          close_stream t;


the previous behavior flushed the writer before marking the stream as closed. Was this change intentional?

Here's the recent history for this line of code

I added the flush call with a callback to Stream.finish_stream in d7e5795#diff-71bafa60481b1abadc94a8c1599b45c6R492 (tests would fail otherwise)

you made the Stream.finish_stream a close_stream call in b126bb2#diff-71bafa60481b1abadc94a8c1599b45c6R488

since close_stream already flushes (either directly, or via reset_stream), I removed the explicit flush call

dune runtest and my toy gRPC project are running fine, and a quick look at the code makes me think it should work, but I'll defer to you since you're more experienced with the codebase.

Good point -- I forgot close_stream already flushed.

anmonteiro

@blandinw thank you for addressing my comments. this looks almost ready now! I just left one additional question.

anmonteiro · 2020-09-28T22:30:52Z

Thanks again!

blandinw · 2020-09-28T22:32:15Z

Happy to contribute, it was fun 😄

anmonteiro mentioned this pull request Sep 27, 2020

Move streaming flush out of body #142

Merged

blandinw force-pushed the master branch from e24d2db to a842d03 Compare September 27, 2020 11:38

anmonteiro force-pushed the master branch from a842d03 to f3953a1 Compare September 27, 2020 23:34

anmonteiro reviewed Sep 28, 2020

View reviewed changes

allow trailing headers, fixes anmonteiro#97

b7e7df7

blandinw force-pushed the master branch from f3953a1 to 567e554 Compare September 28, 2020 13:04

blandinw commented Sep 28, 2020

View reviewed changes

@anmonteiro

a0233f5

blandinw force-pushed the master branch from 567e554 to a0233f5 Compare September 28, 2020 13:06

blandinw changed the title ~~add Reqd.send_trailers_on_close (fixes #97)~~ add Reqd.schedule_trailers (fixes #97) Sep 28, 2020

anmonteiro reviewed Sep 28, 2020

View reviewed changes

anmonteiro approved these changes Sep 28, 2020

View reviewed changes

anmonteiro merged commit e92be08 into anmonteiro:master Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Reqd.schedule_trailers (fixes #97) #141

add Reqd.schedule_trailers (fixes #97) #141

blandinw commented Sep 27, 2020 •

edited

anmonteiro commented Sep 27, 2020

blandinw commented Sep 27, 2020 •

edited

anmonteiro commented Sep 27, 2020

anmonteiro left a comment

anmonteiro Sep 28, 2020

blandinw Sep 28, 2020

anmonteiro Sep 28, 2020

blandinw Sep 28, 2020

anmonteiro Sep 28, 2020

blandinw left a comment

blandinw Sep 28, 2020

blandinw Sep 28, 2020

anmonteiro Sep 28, 2020

blandinw Sep 28, 2020 •

edited

anmonteiro Sep 28, 2020

anmonteiro left a comment

anmonteiro commented Sep 28, 2020

blandinw commented Sep 28, 2020

add Reqd.schedule_trailers (fixes #97) #141

add Reqd.schedule_trailers (fixes #97) #141

Conversation

blandinw commented Sep 27, 2020 • edited

anmonteiro commented Sep 27, 2020

blandinw commented Sep 27, 2020 • edited

anmonteiro commented Sep 27, 2020

anmonteiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blandinw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blandinw Sep 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro left a comment

Choose a reason for hiding this comment

anmonteiro commented Sep 28, 2020

blandinw commented Sep 28, 2020

blandinw commented Sep 27, 2020 •

edited

blandinw commented Sep 27, 2020 •

edited

blandinw Sep 28, 2020 •

edited