DATA frames are sent after RST_STREAM frames

[jonathanjameswatson]

I have experienced a couple situations where the following NYI exception is raised on the client:

ocaml-h2/lib/respd.ml

Line 147 in 5071a6e

failwith "h2.Reqd.report_exn: NYI"

This seems to occur due to DATA frames being sent by the server to the client after it has already sent an RST_STREAM frame, which should not be followed by any other type of frame.

One circumstance in which this happens is where Reqd.report_exn is called after Reqd.respond_with_string. This works with http/af, but in h2, the resulting HEADERS, DATA and RST_STREAM frames are delivered in an incorrect order. I can fix this by commenting out the following line:

ocaml-h2/lib/reqd.ml

Line 387 in 5071a6e

reset_stream t error_code

The RST_STREAM frame is still sent, but after the other two frames. I am not sure if this is the correct solution to the problem.

I have also run into this issue when sending a request that has the content-length header set to a string like "Not a number". The above fix does not work in this case, although this is not unexpected as reset_stream is found in a lot of places. I think either reset_steam needs to be changed, what happens after reset_stream is called needs to be changed, or reset_stream needs to be removed.

[anmonteiro]

I'll look into this in a bit, but it'd be great to have either a reproduction, or ideally a test that exposes this bug in functionality.

[jonathanjameswatson]

Here is the test I started writing for when the content length is not a number:

let test_non_number_content_length_with_data () =
  let body_read_called = ref false in
  let request =
    Request.create
      ~scheme:"http"
      `GET
      "/"
      ~headers:(Headers.of_list [ "content-length", "not-a-number" ])
  in
  let request_handler reqd =
    let request_body = Reqd.request_body reqd in
    Body.Reader.schedule_read
      request_body
      ~on_eof:ignore
      ~on_read:(fun _bs ~off:_ ~len:_ -> body_read_called := true)
  in
  let t = create_and_handle_preface ~error_handler request_handler in
  read_request ~body:"request body" t request;
  let data_frame =
    { Frame.frame_header =
        { payload_length = 0
        ; stream_id = 1l
        ; flags = Flags.(default_flags |> set_end_stream)
        ; frame_type = Data
        }
    ; frame_payload = Frame.Data (Bigstringaf.of_string ~off:0 ~len:3 "foo")
    }
  in
  read_frames t [ data_frame ];
  match next_write_operation t with
  | `Write iovecs ->
    let frames = parse_frames (Write_operation.iovecs_to_string iovecs) in
    Alcotest.(check (list int))
      "???"
      (List.map Frame.FrameType.serialize Frame.FrameType.[ RSTStream ])
      (* Or maybe [ Headers; Data ]? *)
      (List.map
         (fun Frame.{ frame_header = { frame_type; _ }; _ } ->
           Frame.FrameType.serialize frame_type)
         frames);
    report_write_result t (`Ok (IOVec.lengthv iovecs));
    Alcotest.(check bool) "Response handler called" true !body_read_called;
    writer_yields t
  | _ -> assert false

It's not a great example, as arguably an RST_STREAM frame may not need to be output at all. However, it does demonstrate the common behavior where DATA frames are output after RST_STREAM frames.

Here is a test I have written for the first problem:

let test_reset_stream () =
  let request = Request.create ~scheme:"http" `GET "/" in
  let request_handler reqd =
    Reqd.respond_with_string
      reqd
      (Response.create `Internal_server_error)
      "An error occurred";
    Reqd.report_exn reqd Not_found
  in
  let t = create_and_handle_preface ~error_handler request_handler in
  read_request ~body:"request body" t request;
  match next_write_operation t with
  | `Write iovecs ->
    let frames = parse_frames (Write_operation.iovecs_to_string iovecs) in
    Alcotest.(check (list int))
      "???"
      (List.map
         Frame.FrameType.serialize
         Frame.FrameType.[ Headers; Data; RSTStream ])
      (* Not entirely sure what frames should be received here. *)
      (List.map
         (fun Frame.{ frame_header = { frame_type; _ }; _ } ->
           Frame.FrameType.serialize frame_type)
         frames);
    report_write_result t (`Ok (IOVec.lengthv iovecs));
    writer_yields t
  | _ -> assert false

However, it may be that using report_exn after respond_with_string is not a correct use of Reqd, and it just happens to not cause any problems when used with http/af.

[anmonteiro]

Thanks for the repro. I added a fix for the server in #194. Will take a look at the client portion soon