Support upgrades via I/O operations #159
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seliopou
commented
Oct 15, 2019
| @@ -34,6 +34,7 @@ | |||
|
|
|||
| open Lwt.Infix | |||
|
|
|||
| (* Based on the Buffer module in httpaf_async.ml. *) | |||
|
There's been renewed interest in using
It seems to me that the runtime is not particularly well suited for writing the response to the wire. The code for serialising a response is currently kept in I'm wondering whether a better invariant would be for Based on those semantics of It could easily be the case I'm missing something, but on the other hand maybe this feedback is useful. Regardless, I'd be happy to see upgrade support in |
|
Hey @andreas, this has been on our minds as well recently. My plan was to spend some time working on this this weekend, and I'll definitely take a look at your PR. The example programs are especially appreciated! |
dpatti
added a commit
that referenced
this pull request
Apr 12, 2020
This is a prelude to #159 which introduces streaming requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
dpatti
added a commit
that referenced
this pull request
Apr 12, 2020
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
anmonteiro
referenced
this pull request
in anmonteiro/httpun
Apr 18, 2020
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
Add `Reqd.respond_with_upgrade`, which will create response with a with status code 101, and the provided headers. On the next write operation, this response, along with the inspiring request will be provided to the runtime. It is up to the runtime to serialize the resposne and send it on the wire before handing off the socket to an upgrade handler.
The future returned by the ugprade handler will dicatate when the fd will be closed.
The deferred returned by the ugprade handler will dicatate when the fd will be closed.
|
@andreas I force-pushed to this branch so you may want to update your fork. I'm going to try and add some more tests so I can merge #172 and then get this in. Do you think this approach is workable in general? I'm hoping that something like this can also be adapted for use with sendfile, and other such things. |
dpatti
added a commit
that referenced
this pull request
May 6, 2020
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
Thanks for the heads up. I've updated my branch to be based on the latest version of this branch (upgrade...andreas:upgrade2).
I like the approach of signalling an upgrade to the runtime. To recap, these are the two different approaches, as I understand them: A. The semantics suggested in this PR, where Do I understand correctly, that you prefer (A) because it would more easily support something like sendfile? |
|
Yeah, my question was regarding how the upgrade is signaled to the runtime. I don't quite know which of A or B to do just yet. We're changing a bunch of stuff related to the server connection module and underlying machinery. Once that work's complete I'll rebase this and make a final determination. But I'd say as long as the state machine changes for flushing the write and then surfacing the upgrade is simple and straightforward, I wouldn't be opposed to that. Probably when I first wrote this I either thought it was more trouble than it's worth or actually bumped into some issue that made it problematic. We'll see once more once the dust settles. |
|
Makes sense — looking forward to see how it works out 🙂 |
dpatti
added a commit
that referenced
this pull request
May 16, 2020
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
seliopou
pushed a commit
that referenced
this pull request
May 19, 2020
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
dpatti
added a commit
that referenced
this pull request
Apr 2, 2021
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
dpatti
added a commit
that referenced
this pull request
Apr 2, 2021
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
dpatti
added a commit
that referenced
this pull request
Apr 3, 2021
This is a prelude to #159 which introduces upgrade requests, and much of the diff is identical, with a few trivial changes in `Reqd` and a few major changes in `Server_connection`. The goals here are: 1. Make `Reqd` return better types that encapsulate its state instead of requiring the user to probe it with `requires_<input|output>` and `is_complete`. 2. 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 `yield_<reader|writer>` and `next_<read|write>_operation` functions very parallel. For one, the extra logic in `yield_writer` was puzzling. Ideally, if you're calling `yield_writer`, you're doing so because you just called `next_action` and were told to `Yield`, so all of the conditions being checked should not be possible. Looking at the next-operation functions, they both start 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. When we do shift it off, we recursively ask for the next operation given the new queue state. In all cases, before we return the result, we wakeup the other side so that it too can evaluate the next operation given the new queue state. 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. I added a `Queue.clear` to shutdown, not because it was necessary in any sense, but because it was part of `advance_request_queue_if_necessary`, which could have come into play in certain situations where `shutdown` was called from the runtime (e.g., in case of some exception). I would like to note that despite the fact that all tests pass, I have very little confidence in this being correct right now and would like to do some further testing within the actual runtimes.
dpatti
added a commit
that referenced
this pull request
Apr 3, 2021
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.
dpatti
added a commit
that referenced
this pull request
Apr 22, 2021
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.
dpatti
added a commit
that referenced
this pull request
Apr 22, 2021
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.
Open
|
Hi folks. This PR no longer applies at all cleanly after #172. However, I have tried to recreate a PR with basically the same design in #203. My PR takes Option B as described by @andreas. This seemed like the much easier method to me, especially as, as was pointed out, Httpaf provides no other way for the client application to write the response to the wire. |
dpatti
added a commit
that referenced
this pull request
Apr 26, 2021
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
Reqd.respond_with_upgrade, which will create response with a with status code 101, and the provided headers. On the next write operation, this response, along with the inspiring request will be provided to the runtime. It is up to the runtime to serialize the resposne and send it on the wire before handing off the socket to an upgrade handler.The runtime code almost certainly has bugs in it, so this isn't going to be merged immediately. Specifically, I'm pretty sure that both in async and lwt the sockets will be closed right after the upgrade handlers are called. Also, neither currently serialize the response on the wire.
Subsumes #134.