refactor request queue mechanics #172

dpatti · 2020-04-12T19:16:13Z

This is a prelude to #159 which introduces upgrade requests, with a few
major changes in Server_connection.

The goals here is to try to make queue management easier to reason about
by folding bits of logic from advance_request_queue_if_necessary into
next_read_operation and next_write_operation such that we only
perform side-effects when the operation in question demands it.

One of the ways I tried to make this easier to reason about was to make
the next_<read|write>_operation functions very parallel. Getting the
read operation starts out with a short-circuit for shutting down when
the server can no longer make progress (reader is closed and queue is
empty). This doesn't feel like it belongs here. Perhaps this check
should be part of advance_request_queue with some extra logic
triggering in shutdown_reader? After that, the next-operation
functions use some very simple probing of the input/output state of
Reqd to determine what to do next. Only in the case of Complete do
we move into a separate function (to make it easier to read):
_final_<read|write>_operation.

In these functions, we decide if we should shutdown the respective
reader/writer or consider the reqd complete and move it off the queue.
What's happening is that we don't know if the write action or read
action will be last, so each function checks the state of the other to
see if they're both complete. When we do shift it off, we recursively
ask for the next operation given the new queue state.

In the case of the writer triggering the advancing, before we return the
result, we wakeup the reader so that it can evaluate the next operation
given the new queue state.

Note that in the case of a non-persistent connection, the queue is never
advanced and the connection is shut down when both sides are done.

Though on the surface, these pieces feel fairly straightforward, there
are still a slew of re-entrancy bugs to consider. I think there are two
things that we can do to make this drastically easier to manage:

We call t.request_handler in two places, and this is mostly because
we want to keep the invariant that the head of the request queue has
already been passed off to the handler. I feel like splitting this up
into a simple queue of unhandled requests and a [Reqd.t option] that
represents the current request would be easier to manage.
It would be nice to schedule calls. Things like waking up the writer
before you let the read loop know its next operation just immediately
makes my mind fall apart and lose track of state. There's a fairly
obvious solution of asking for a schedule : (unit -> unit) -> unit
function from the runtime that promises to not call the thunk
synchronously, but rather waits until it is outside of the read and
write loops. But maybe we can solve it using what we have now, like
establishing a contract that when the reader/writer is woken up, they
must schedule their work for a fresh call stack and not immediately
ask for operations.

lib/server_connection.ml

These are just the reqd changes from #172

This is part of #172

Part of #172, but changed a constructor name.

seliopou · 2020-05-17T15:13:53Z

lib/server_connection.ml

+      Writer.next t.writer;
+    ) else (
+      match Reqd.input_state reqd with
+      | Ready -> assert false


Shouldn't this just be Writer.next t.writer? A request handler could be done writing a response while still reading a request. Not sure of the wisdom of writing a request handler that does that but it is possible.

That's a good point. This makes me think that a good way to improve testing would be to checkout master and just randomly delete lines from server_connection.ml to see what still passes. Then we can either write a test that makes it fail or decide it wasn't necessary.

This is fixed. See last commit for details.

seliopou · 2020-05-19T11:25:58Z

lib/server_connection.ml

@@ -155,6 +155,7 @@ let error_code t =
  else None

 let shutdown t =
+  Queue.clear t.request_queue;


If there is an active connection, this will cause any active request handlers to hang on reading from the request body.

Yeah I wasn't thrilled by this. Definitely worth revisiting and writing more tests for.

This was removed. See last commit for details.

dpatti · 2021-04-03T23:29:23Z

I rewrote the PR description (and first commit message) since they were both out of date after we pulled even more chunks off. I'm feeling pretty good about this now.

seliopou · 2021-04-22T12:59:02Z

lib/server_connection.ml

  )
-  else Reader.next t.reader


Read looks good.

seliopou · 2021-04-22T13:03:07Z

lib/server_connection.ml

+  next
+;;
+
+let next_write_operation t = _next_write_operation t


Writer looks good.

This is a prelude to #159 which introduces upgrade requests, with a few major changes in `Server_connection`. The goals here is to try to make queue management easier to reason about by folding bits of logic from `advance_request_queue_if_necessary` into `next_read_operation` and `next_write_operation` such that we only perform side-effects when the operation in question demands it. One of the ways I tried to make this easier to reason about was to make the `next_<read|write>_operation` functions very parallel. Getting the read operation starts out with a short-circuit for shutting down when the server can no longer make progress (reader is closed and queue is empty). This doesn't feel like it belongs here. Perhaps this check should be part of `advance_request_queue` with some extra logic triggering in `shutdown_reader`? After that, the next-operation functions use some very simple probing of the input/output state of `Reqd` to determine what to do next. Only in the case of `Complete` do we move into a separate function (to make it easier to read): `_final_<read|write>_operation`. In these functions, we decide if we should shutdown the respective reader/writer or consider the `reqd` complete and move it off the queue. What's happening is that we don't know if the write action or read action will be last, so each function checks the state of the other to see if they're both complete. When we do shift it off, we recursively ask for the next operation given the new queue state. In the case of the writer triggering the advancing, before we return the result, we wakeup the reader so that it can evaluate the next operation given the new queue state. Note that in the case of a non-persistent connection, the queue is never advanced and the connection is shut down when both sides are done. Though on the surface, these pieces feel fairly straightforward, there are still a slew of re-entrancy bugs to consider. I think there are two things that we can do to make this drastically easier to manage: 1. We call `t.request_handler` in two places, and this is mostly because we want to keep the invariant that the head of the request queue has already been passed off to the handler. I feel like splitting this up into a simple queue of unhandled requests and a [Reqd.t option] that represents the current request would be easier to manage. 2. It would be nice to schedule calls. Things like waking up the writer before you let the read loop know its next operation just immediately makes my mind fall apart and lose track of state. There's a fairly obvious solution of asking for a `schedule : (unit -> unit) -> unit` function from the runtime that promises to not call the thunk synchronously, but rather waits until it is outside of the read and write loops. But maybe we can solve it using what we have now, like establishing a contract that when the reader/writer is woken up, they must schedule their work for a fresh call stack and not immediately ask for operations.

This is because the writer is always woken by the appropriate calls that push chunks onto the body or writer or calls that close the body. Had to import an additional line from a recent band-aid fix regarding setting the flag on non-chunked streaming responses. It feels like we should find an alternative means of maintaining this piece of information.

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.

seliopou · 2021-05-16T21:08:28Z

Is this good? I think this is good.

dpatti · 2021-05-22T18:22:08Z

@seliopou I think we should release this. We don't have to tag until more of the other features are released, and having this in master would definitely simplify the development story.

dpatti requested a review from seliopou April 12, 2020 19:16

anuragsoni reviewed Apr 13, 2020

View reviewed changes

lib/server_connection.ml Outdated Show resolved Hide resolved

seliopou mentioned this pull request Apr 27, 2020

Support upgrades via I/O operations #159

Open

seliopou pushed a commit that referenced this pull request Apr 28, 2020

refactor request queue mechanics

91e84d4

These are just the reqd changes from #172

seliopou added a commit that referenced this pull request Apr 28, 2020

response-state: Put Reqd.response_state in its own module

76293ce

This is part of #172

seliopou mentioned this pull request Apr 28, 2020

response-state: Put Reqd.response_state in its own module #173

Merged

seliopou added a commit that referenced this pull request Apr 28, 2020

input-state: use a type to indicate Reqd input state

e05f072

Part of #172, but changed a constructor name.

seliopou added a commit that referenced this pull request Apr 28, 2020

input-state: use a type to indicate Reqd input state

c57c2be

Part of #172, but changed a constructor name.

seliopou mentioned this pull request Apr 28, 2020

input-state: use a type to indicate Reqd input state #174

Merged

dpatti force-pushed the refactor-request-queue branch from d89b750 to 7d211fb Compare May 6, 2020 22:12

dpatti force-pushed the refactor-request-queue branch from 7d211fb to 39d8c7b Compare May 16, 2020 19:09

seliopou reviewed May 17, 2020

View reviewed changes

seliopou force-pushed the refactor-request-queue branch from 94db517 to e8386ff Compare May 19, 2020 11:23

seliopou requested changes May 19, 2020

View reviewed changes

dpatti force-pushed the refactor-request-queue branch 2 times, most recently from 568a2d8 to 9c526e7 Compare April 2, 2021 20:28

dpatti changed the base branch from master to test-requests-queued-at-close April 2, 2021 20:29

dpatti force-pushed the test-requests-queued-at-close branch 2 times, most recently from cfa3085 to e8e8f89 Compare April 3, 2021 23:14

dpatti force-pushed the refactor-request-queue branch 2 times, most recently from 4ca78f2 to 6b14105 Compare April 3, 2021 23:27

dpatti requested a review from seliopou April 3, 2021 23:28

Base automatically changed from test-requests-queued-at-close to master April 4, 2021 21:31

seliopou reviewed Apr 22, 2021

View reviewed changes

lib/server_connection.ml

)

else Reader.next t.reader

Copy link

Member

seliopou Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read looks good.

seliopou reviewed Apr 22, 2021

View reviewed changes

lib/server_connection.ml

next

;;

let next_write_operation t = _next_write_operation t

Copy link

Member

seliopou Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writer looks good.

dpatti force-pushed the refactor-request-queue branch 4 times, most recently from 3178faa to cce55fd Compare April 22, 2021 19:23

dhouse-js mentioned this pull request Apr 26, 2021

Http upgrades #203

Open

dpatti force-pushed the refactor-request-queue branch from cce55fd to 77f216b Compare April 26, 2021 19:58

dpatti added 3 commits April 26, 2021 16:02

seliopou mentioned this pull request May 16, 2021

Process requests after write EOF #209

Open

seliopou merged commit cc7478a into master May 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor request queue mechanics #172

refactor request queue mechanics #172

dpatti commented Apr 12, 2020 •

edited

seliopou May 17, 2020

dpatti May 23, 2020

dpatti Apr 2, 2021

seliopou May 19, 2020

dpatti May 23, 2020

dpatti Apr 2, 2021

dpatti commented Apr 3, 2021

seliopou Apr 22, 2021

seliopou Apr 22, 2021

seliopou commented May 16, 2021

dpatti commented May 22, 2021

refactor request queue mechanics #172

refactor request queue mechanics #172

Conversation

dpatti commented Apr 12, 2020 • edited

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

dpatti commented Apr 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seliopou commented May 16, 2021

dpatti commented May 22, 2021

dpatti commented Apr 12, 2020 •

edited