refactor-request-queue: fixes

We basically never want to call `Queue.clear` because the head of the
queue has special semantic meaning. Instead, we never try to clear the
queue and rely on the fact that the queue will never be advanced. This
is easy to reason about because the only time we advance the request
queue is when the current request is not persistent. I added an explicit
test of this situation to build confidence.

Additionally, there was an incorrect assertion that you couldn't finish
a write with reads still pending. A test was added upstream and it no
longer fails with this fix.

The final change was some subtle but unused code. In the write loop, we
have something that decides to shutdown the connection if the reader is
closed, parallel to the next read operation. But this felt weird, the
reader should always be awake in the case that it is closed, which means
that either 1) it will shutdown the connection or 2) it will wait for
the writer, which will wake the reader once it's advanced the request
queue, and then it will shutdown the connection.

Author: dpatti

lib/server_connection.ml

@@ -155,7 +155,6 @@ let error_code t =
   else None
 
 let shutdown t =
-  Queue.clear t.request_queue;
   shutdown_reader t;
   shutdown_writer t;
   wakeup_reader t;
@@ -190,7 +189,8 @@ let advance_request_queue t =
 ;;
 
 let rec _next_read_operation t =
-  if not (is_active t) then (
+  if not (is_active t)
+  then (
     if Reader.is_closed t.reader
     then shutdown t;
     Reader.next t.reader
@@ -216,7 +216,6 @@ and _final_read_operation_for t reqd =
 
 let next_read_operation t =
   match _next_read_operation t with
-  (* XXX(dpatti): These two [`Error _] constructors are never returned *)
   | `Error (`Parse _)             -> set_error_and_handle          t `Bad_request; `Close
   | `Error (`Bad_request request) -> set_error_and_handle ~request t `Bad_request; `Close
   | (`Read | `Yield | `Close) as operation -> operation
@@ -248,11 +247,9 @@ let read_eof t bs ~off ~len =
   read_with_more t bs ~off ~len Complete
 
 let rec _next_write_operation t =
-  if not (is_active t) then (
-    if Reader.is_closed t.reader
-    then shutdown t;
-    Writer.next t.writer
-  ) else (
+  if not (is_active t)
+  then Writer.next t.writer
+  else (
     let reqd = current_reqd_exn t in
     match Reqd.output_state reqd with
     | Waiting ->
@@ -274,7 +271,7 @@ and _final_write_operation_for t reqd =
       Writer.next t.writer;
     ) else (
       match Reqd.input_state reqd with
-      | Ready -> assert false
+      | Ready -> Writer.next t.writer;
       | Complete ->
         advance_request_queue t;
         _next_write_operation t;

lib_test/test_server_connection.ml

@@ -647,7 +647,7 @@ let test_multiple_requests_in_single_read () =
     request_to_string (Request.create `GET "/")
   in
   read_string t reqs;
-  reader_yielded t;
+  reader_ready t;
   Alcotest.(check int) "fired handler of both requests" 2 !reqs_handled
 ;;