From d1cc26f4626f2fcff93eff0d8a4af603e8133162 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Fri, 17 Nov 2023 10:45:16 -0800
Subject: [PATCH] Add more documentation for the FFI API

---
 src/ffi/client.rs |  36 +++++++++++++--
 src/ffi/io.rs     |  39 ++++++++++++-----
 src/ffi/task.rs   | 109 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 166 insertions(+), 18 deletions(-)

diff --git a/src/ffi/client.rs b/src/ffi/client.rs
index 4de81d9b06..8061d1c4fd 100644
--- a/src/ffi/client.rs
+++ b/src/ffi/client.rs
@@ -23,9 +23,39 @@ pub struct hyper_clientconn_options {
 
 /// An HTTP client connection handle.
 ///
-/// These are used to send a request on a single connection. It's possible to
-/// send multiple requests on a single connection, such as when HTTP/1
-/// keep-alive or HTTP/2 is used.
+/// These are used to send one or more requests on a single connection.
+///
+/// It's possible to send multiple requests on a single connection, such
+/// as when HTTP/1 keep-alive or HTTP/2 is used.
+///
+/// To create a hyper_clientconn:
+///
+///   1. Create a hyper_clientconn_options with hyper_clientconn_options_new.
+///   2. Create a hyper_io with hyper_io_new.
+///   3. Call hyper_clientconn_handshake with the hyper_io and hyper_clientconn_options.
+///      This creates a hyper_task.
+///   4. Add the hyper_task to an executor with hyper_executor_push.
+///   5. Poll that executor until it yields a task of type HYPER_TASK_CLIENTCONN.
+///   6. Extract the hyper_clientconn from the task with hyper_task_value.
+///      This will require a cast from void * to hyper_clientconn *.
+///
+/// This process results in a hyper_clientconn that permanently owns the
+/// hyper_io. Because the hyper_io in turn owns a TCP or TLS connection, that means
+/// the hyper_clientconn owns the connection for both the clientconn's lifetime
+/// and the connection's lifetime.
+///
+/// In other words, each connection (hyper_io) must have exactly one hyper_clientconn
+/// associated with it. That's because hyper_clientconn_handshake sends the
+/// [HTTP/2 Connection Preface] (for HTTP/2 connections). Since that preface can't
+/// be sent twice, handshake can't be called twice.
+///
+/// [HTTP/2 Connection Preface]: https://datatracker.ietf.org/doc/html/rfc9113#name-http-2-connection-preface
+///
+/// Methods:
+///
+/// hyper_clientconn_free       Free a hyper_clientconn *.
+/// hyper_clientconn_handshake  Starts an HTTP client connection handshake using the provided IO transport and options.
+/// hyper_clientconn_send       Send a request on the client connection.
 pub struct hyper_clientconn {
     tx: Tx,
 }
diff --git a/src/ffi/io.rs b/src/ffi/io.rs
index c1ba87a02b..7655897fa0 100644
--- a/src/ffi/io.rs
+++ b/src/ffi/io.rs
@@ -19,7 +19,20 @@ type hyper_io_read_callback =
 type hyper_io_write_callback =
     extern "C" fn(*mut c_void, *mut hyper_context<'_>, *const u8, size_t) -> size_t;
 
-/// An IO object used to represent a socket or similar concept.
+/// A read/write handle for a specific connection.
+///
+/// This owns a specific TCP or TLS connection for the lifetime of
+/// that connection. It contains a read and write callback, as well as a
+/// void *userdata. Typically the userdata will point to a struct
+/// containing a file descriptor and a TLS context.
+///
+/// Methods:
+///
+/// hyper_io_new          Create a new IO type used to represent a transport.
+/// hyper_io_set_read     Set the read function for this IO transport.
+/// hyper_io_set_userdata Set the user data pointer for this IO to some value.
+/// hyper_io_set_write    Set the write function for this IO transport.
+/// hyper_io_free         Free an IO handle.
 pub struct hyper_io {
     read: hyper_io_read_callback,
     write: hyper_io_write_callback,
@@ -32,6 +45,11 @@ ffi_fn! {
     /// The read and write functions of this transport should be set with
     /// `hyper_io_set_read` and `hyper_io_set_write`.
     ///
+    /// It is expected that the underlying transport is non-blocking. When
+    /// a read or write callback can't make progress because there is no
+    /// data available yet, it should use the `hyper_waker` mechanism to
+    /// arrange to be called again when data is available.
+    ///
     /// To avoid a memory leak, the IO handle must eventually be consumed by
     /// `hyper_io_free` or `hyper_clientconn_handshake`.
     fn hyper_io_new() -> *mut hyper_io {
@@ -72,10 +90,11 @@ ffi_fn! {
     /// unless you have already written them yourself. It is also undefined behavior
     /// to return that more bytes have been written than actually set on the `buf`.
     ///
-    /// If there is no data currently available, a waker should be claimed from
-    /// the `ctx` and registered with whatever polling mechanism is used to signal
-    /// when data is available later on. The return value should be
-    /// `HYPER_IO_PENDING`.
+    /// If there is no data currently available, the callback should create a
+    /// `hyper_waker` from its `hyper_context` argument and register the waker
+    /// with whatever polling mechanism is used to signal when data is available
+    /// later on. The return value should be `HYPER_IO_PENDING`. See the
+    /// documentation for `hyper_waker`.
     ///
     /// If there is an irrecoverable error reading data, then `HYPER_IO_ERROR`
     /// should be the return value.
@@ -90,11 +109,11 @@ ffi_fn! {
     /// Data from the `buf` pointer should be written to the transport, up to
     /// `buf_len` bytes. The number of bytes written should be the return value.
     ///
-    /// If no data can currently be written, the `waker` should be cloned and
-    /// registered with whatever polling mechanism is used to signal when data
-    /// is available later on. The return value should be `HYPER_IO_PENDING`.
-    ///
-    /// Yeet.
+    /// If there is no data currently available, the callback should create a
+    /// `hyper_waker` from its `hyper_context` argument and register the waker
+    /// with whatever polling mechanism is used to signal when data is available
+    /// later on. The return value should be `HYPER_IO_PENDING`. See the documentation
+    /// for `hyper_waker`.
     ///
     /// If there is an irrecoverable error reading data, then `HYPER_IO_ERROR`
     /// should be the return value.
diff --git a/src/ffi/task.rs b/src/ffi/task.rs
index 78e92ba90c..0f2397afa2 100644
--- a/src/ffi/task.rs
+++ b/src/ffi/task.rs
@@ -28,6 +28,28 @@ pub const HYPER_POLL_PENDING: c_int = 1;
 pub const HYPER_POLL_ERROR: c_int = 3;
 
 /// A task executor for `hyper_task`s.
+///
+/// A task is a unit of work that may be blocked on IO, and can be polled to
+/// make progress on that work.
+///
+/// An executor can hold many tasks, included from unrelated HTTP connections.
+/// An executor is single threaded. Typically you might have one executor per
+/// thread. Or, for simplicity, you may choose one executor per connection.
+///
+/// Progress on tasks happens only when hyper_executor_poll is called, and only
+/// on tasks whose corresponding `hyper_waker` has been called to indicate they
+/// are ready to make progress (for instance, because the OS has indicated there
+/// is more data to read or more buffer space available to write).
+///
+/// Deadlock potential: hyper_executor_poll must not be called from within a task's
+/// callback. Doing so will result in a deadlock.
+///
+/// Methods:
+///
+/// hyper_executor_new  Creates a new task executor.
+/// hyper_executor_push Push a task onto the executor.
+/// hyper_executor_poll Polls the executor, trying to make progress on any tasks that have notified that they are ready again.
+/// hyper_executor_free Frees an executor and any incomplete tasks still part of it.
 pub struct hyper_executor {
     /// The executor of all task futures.
     ///
@@ -55,6 +77,38 @@ pub(crate) struct WeakExec(Weak<hyper_executor>);
 struct ExecWaker(AtomicBool);
 
 /// An async task.
+///
+/// A task represents a chunk of work that will eventually yield a hyper_task_value.
+/// Tasks are pushed onto an executor, and that executor is responsible for calling
+/// the necessary functions on the task to make progress.
+///
+/// Tasks are created by various functions:
+///
+/// hyper_clientconn_handshake Starts an HTTP client connection handshake.
+/// hyper_clientconn_send      Send a request on the client connection.
+/// hyper_body_data            Return a task that will poll the body for data.
+/// hyper_body_foreach         Return a task that will poll the body and execute a callback.
+///
+/// Tasks then have a userdata associated with them using hyper_task_set_userdata. This
+/// is important, for instance, to associate a request id with a given request. When multiple
+/// tasks are running on the same executor, this allows distinguishing tasks for different
+/// requests.
+///
+/// Tasks are then pushed onto an executor, and eventually yielded from hyper_executor_poll:
+///
+/// hyper_executor_push        Push a task onto the executor.
+/// hyper_executor_poll        Polls the executor, trying to make progress on any tasks that have notified that they are ready again.
+///
+/// Once a task is yielded from poll, retrieve its userdata, check its type,
+/// and extract its value. This will require a case from void* to the appropriate type.
+///
+/// Methods on hyper_task:
+///
+/// hyper_task_type            Query the return type of this task.
+/// hyper_task_value           Takes the output value of this task.
+/// hyper_task_set_userdata    Set a user data pointer to be associated with this task.
+/// hyper_task_userdata        Retrieve the userdata that has been set via hyper_task_set_userdata.
+/// hyper_task_free            Free a task.
 pub struct hyper_task {
     future: BoxFuture<BoxAny>,
     output: Option<BoxAny>,
@@ -66,9 +120,35 @@ struct TaskFuture {
 }
 
 /// An async context for a task that contains the related waker.
+///
+/// This is provided to hyper_io's read and write callbacks. Currently
+/// its only purpose is to provide access to the waker. See hyper_waker.
+///
+/// Corresponding Rust type: <https://doc.rust-lang.org/std/task/struct.Context.html>
 pub struct hyper_context<'a>(Context<'a>);
 
 /// A waker that is saved and used to waken a pending task.
+///
+/// This is provided to hyper_io's read and write callbacks via hyper_context.
+///
+/// When nonblocking I/O in one of those callbacks can't make progress (returns
+/// EAGAIN or EWOULDBLOCK), the callback has to return to avoid blocking the
+/// executor. But it also has to arrange to get called in the future when more
+/// data is available. That's the role of the async context and the waker. The
+/// waker can be used to tell the executor "this task is ready to make progress."
+///
+/// The read or write callback, upon finding it can't make progress, must get a
+/// waker from the context (hyper_context_waker), arrange for that waker to be
+/// called in the future, and then return HYPER_POLL_PENDING.
+///
+/// The arrangements for the waker to be called in the future are up to the
+/// application, but usually it will involve one big `select(2)` loop that checks which
+/// FDs are ready, and a correspondence between FDs and waker objects. For each
+/// FD that is ready, the corresponding waker must be called. Then hyper_executor_poll
+/// must be called. That will cause the executor to attempt to make progress on each
+/// woken task.
+///
+/// Corresponding Rust type: <https://doc.rust-lang.org/std/task/struct.Waker.html>
 pub struct hyper_waker {
     waker: std::task::Waker,
 }
@@ -219,8 +299,14 @@ ffi_fn! {
 ffi_fn! {
     /// Push a task onto the executor.
     ///
-    /// The executor takes ownership of the task, which should not be accessed
-    /// again unless returned back to the user with `hyper_executor_poll`.
+    /// The executor takes ownership of the task, which must not be accessed
+    /// again.
+    ///
+    /// Ownership of the task will eventually be returned to the user from
+    /// `hyper_executor_poll`.
+    ///
+    /// To distinguish multiple tasks running on the same executor, use
+    /// hyper_task_set_userdata.
     fn hyper_executor_push(exec: *const hyper_executor, task: *mut hyper_task) -> hyper_code {
         let exec = non_null!(&*exec ?= hyper_code::HYPERE_INVALID_ARG);
         let task = non_null!(Box::from_raw(task) ?= hyper_code::HYPERE_INVALID_ARG);
@@ -236,8 +322,7 @@ ffi_fn! {
     /// If ready, returns a task from the executor that has completed.
     ///
     /// To avoid a memory leak, the task must eventually be consumed by
-    /// `hyper_task_free`, or taken ownership of by `hyper_executor_push`
-    /// without subsequently being given back by `hyper_executor_poll`.
+    /// `hyper_task_free`.
     ///
     /// If there are no ready tasks, this returns `NULL`.
     fn hyper_executor_poll(exec: *const hyper_executor) -> *mut hyper_task {
@@ -341,6 +426,9 @@ ffi_fn! {
     ///
     /// This value will be passed to task callbacks, and can be checked later
     /// with `hyper_task_userdata`.
+    ///
+    /// This is useful for telling apart tasks for different requests that are
+    /// running on the same executor.
     fn hyper_task_set_userdata(task: *mut hyper_task, userdata: *mut c_void) {
         if task.is_null() {
             return;
@@ -414,7 +502,13 @@ impl hyper_context<'_> {
 }
 
 ffi_fn! {
-    /// Copies a waker out of the task context.
+    /// Creates a waker associated with the task context.
+    ///
+    /// The waker can be used to inform the task's executor that the task is
+    /// ready to make progress (using hyper_waker_wake).
+    ///
+    /// Typically this only needs to be called once, but it can be called
+    /// multiple times, returning a new waker each time.
     ///
     /// To avoid a memory leak, the waker must eventually be consumed by
     /// `hyper_waker_free` or `hyper_waker_wake`.
@@ -439,6 +533,11 @@ ffi_fn! {
 ffi_fn! {
     /// Wake up the task associated with a waker.
     ///
+    /// This does not do work towards associated task. Instead, it signals
+    /// to the task's executor that the task is ready to make progress. The
+    /// application is responsible for calling hyper_executor_poll, which
+    /// will in turn do work on all tasks that are ready to make progress.
+    ///
     /// NOTE: This consumes the waker. You should not use or free the waker afterwards.
     fn hyper_waker_wake(waker: *mut hyper_waker) {
         let waker = non_null!(Box::from_raw(waker) ?= ());