Improved documentation comments for nxt_unit.h

[andreivasiliu]

Improved the comments in nxt_unit.h for the C API, based on what I've understood so far from reading Unit's code while following alejandro-colomar's answers.

This is for #738.

[alejandro-colomar]

Hi!

I recently wrote manual pages for libunit. Could you please have a look
at them? Maybe after that, you find that some of this documentation is
duplicate and we can omit it, or maybe you find it still useful, and
want to confirm it.

See #891

Thanks!
Alex

[andreivasiliu]

Will do, although you might have to wait for a weekend.

[alejandro-colomar]

No problem. I will be entertained implementing other features. :)

[andreivasiliu]

Whether this is duplicate or not, I feel like this header file in particular should have comments in it.

Many people nowadays rely on their IDE to provide some documentation for whatever is under the cursor, and people tend to explore APIs by ctrl/cmd-clicking methods to go to their definition.

But either way, I'll leave that up to this project's owners.

[alejandro-colomar]

Hmm, it might be useful; I'll consider it. A question from someone who has never used an IDE (vim user here): Do IDEs suggest manual pages? Or how do you get help/documentation from standard functions and other library calls?

Regarding the patches themselves: some changes fix typos, syntax, or other trivial issues. Would you mind separating those into a patch, which I'd apply already, and leave the other additions in a second patch that would be discussed? If you don't have the time, I may do that myself, if you agree.

Thanks!

[andreivasiliu]

Do IDEs suggest manual pages?

I haven't seen an IDE that does that so far. They are all based on comments and/or doc-comments.

I may do that myself, if you agree.

Please do, and sorry about that. Also feel free to drop or re-attribute anything there.

[ac000]

Hi @andreivasiliu

I've applied the typos and grammatical fixes here leaving the following for further contemplation.

diff --git a/src/nxt_unit.h b/src/nxt_unit.h
index a50046a5..1bec1094 100644
--- a/src/nxt_unit.h
+++ b/src/nxt_unit.h
@@ -119,19 +119,43 @@ struct nxt_unit_request_info_s {
  */
 struct nxt_unit_callbacks_s {
     /*
      * Process request. Unlike all other callbacks, this callback is required
      * and needs to be defined by the application.
+     *
+     * This callback will be called when all request header and body data is
+     * available.  If the data_handler callback is not NULL, then the
+     * request_handler callback may also sometimes be called without body data.
      */
     void     (*request_handler)(nxt_unit_request_info_t *req);
 
+    /*
+     * Data handler. Optional.
+     *
+     * If this is NULL, then the request_handler() callback will only be called
+     * once all the request body data has been received.
+     *
+     * If this is not NULL, then the request workflow is changed such that the
+     * request_handler() callback may sometimes be called with just the request
+     * header data, before the body content data is available.
+     *
+     * The data_handler() callback will be called only if in request_handler()
+     * the available data was less than the request's content_length and the
+     * nxt_unit_request_done() function was not yet called.
+     *
+     * This callback will be called at most once, when all data becomes
+     * available.
+     */
     void     (*data_handler)(nxt_unit_request_info_t *req);
 
-    /* Process websocket frame. */
+    /* Process websocket frame. Optional. */
     void     (*websocket_handler)(nxt_unit_websocket_frame_t *ws);
 
-    /* Connection closed. */
+    /*
+     * Connection closed. Optional. Called only for websockets that were closed
+     * or requests that were aborted.
+     */
     void     (*close_handler)(nxt_unit_request_info_t *req);
 
     /* Add new Unit port to communicate with process pid. Optional. */
     int      (*add_port)(nxt_unit_ctx_t *, nxt_unit_port_t *port);
 
@@ -234,10 +258,13 @@ int nxt_unit_process_port_msg(nxt_unit_ctx_t *ctx, nxt_unit_port_t *port);
 void nxt_unit_done(nxt_unit_ctx_t *);
 
 /*
  * Allocate and initialize a new execution context with a new listen port to
  * process requests in another thread.
+ *
+ * The new context must be deallocated with nxt_unit_done before the old context
+ * is deallocated.
  */
 nxt_unit_ctx_t *nxt_unit_ctx_alloc(nxt_unit_ctx_t *, void *);
 
 /* Initialize port_id, calculate hash. */
 void nxt_unit_port_id_init(nxt_unit_port_id_t *port_id, pid_t pid, uint16_t id);
@@ -273,15 +300,29 @@ int nxt_unit_response_add_content(nxt_unit_request_info_t *req,
     const void* src, uint32_t size);
 
 /*
  * Send the prepared response to the Unit server.  The Response structure is
  * destroyed during this call.
+ *
+ * Asynchronously, the Unit server will attempt to send the data to the client
+ * as soon as it can, using the "Transfer-Encoding: chunked" method, and may
+ * combine chunks for slow-reading clients.  The connection will then remain
+ * open, and more chunks can be scheduled using using nxt_unit_buf_send() and/or
+ * nxt_unit_write_response(), or the connection can be closed with
+ * nxt_unit_request_done().
  */
 int nxt_unit_response_send(nxt_unit_request_info_t *req);
 
 int nxt_unit_response_is_sent(nxt_unit_request_info_t *req);
 
+/*
+ * Allocate a buffer for an additional response chunk to be sent.  Multiple
+ * buffers may be allocated at the same time, and they may be sent or dropped
+ * in any order.
+ *
+ * See nxt_unit_buf_max() for the maximum size that may be requested.
+ */
 nxt_unit_buf_t *nxt_unit_response_buf_alloc(nxt_unit_request_info_t *req,
     uint32_t size);
 
 int nxt_unit_request_is_websocket_handshake(nxt_unit_request_info_t *req);
 
@@ -289,35 +330,93 @@ int nxt_unit_response_upgrade(nxt_unit_request_info_t *req);
 
 int nxt_unit_response_is_websocket(nxt_unit_request_info_t *req);
 
 nxt_unit_request_info_t *nxt_unit_get_request_info_from_data(void *data);
 
+/*
+ * Send and deallocate a response data chunk.  The data is immediately sent to
+ * the client as a chunk using the "Transfer-Encoding: chunked" method.
+ *
+ * If the initial response was not yet sent with nxt_unit_response_send(), this
+ * function will automatically call it.
+ */
 int nxt_unit_buf_send(nxt_unit_buf_t *buf);
 
+/*
+ * Deallocate a response data chunk without sending it.
+ */
 void nxt_unit_buf_free(nxt_unit_buf_t *buf);
 
 nxt_unit_buf_t *nxt_unit_buf_next(nxt_unit_buf_t *buf);
 
+/*
+ * The maximum size that can be requested with nxt_unit_response_buf_alloc().
+ */
 uint32_t nxt_unit_buf_max(void);
 
+/*
+ * The minimum size that will be allocated by nxt_unit_response_buf_alloc().
+ */
 uint32_t nxt_unit_buf_min(void);
 
+/*
+ * Schedule a response to be sent to the client.  This will repeatedly call
+ * nxt_unit_response_write_nb with a min_size equal to the size, which will
+ * block until the Unit server has received the entire data.
+ *
+ * The Unit server will buffer the response data, and will attempt to send it to
+ * the client asynchronously, as soon as it can.
+ */
 int nxt_unit_response_write(nxt_unit_request_info_t *req, const void *start,
     size_t size);
 
+/*
+ * Schedule a response to be sent to the client, blocking until at least
+ * min_size bytes have been received by the Unit server.
+ */
 ssize_t nxt_unit_response_write_nb(nxt_unit_request_info_t *req,
     const void *start, size_t size, size_t min_size);
 
+/*
+ * Schedule a response to be sent to the client, using a user-provided callback
+ * that will be called repeatedly with buffers to write to.  This function will
+ * return once the Unit server has received all the data.
+ */
 int nxt_unit_response_write_cb(nxt_unit_request_info_t *req,
     nxt_unit_read_info_t *read_info);
 
+/*
+ * Read bytes from the request body.  This is non-blocking.  This function will
+ * return 0 when no more data can be received from the Unit server in the
+ * current request handler callback.
+ *
+ * If the data_handler callback is NULL, then the Unit server will already have
+ * the entire request body data buffered, and this function can receive the
+ * whole request.
+ *
+ * If the data_handler callback is not NULL, then:
+ *
+ * 1. Inside request_handler(), this function may sometimes return 0 before the
+ *    amount of received data reaches the request's content_length.  In this
+ *    case, Unit will call the data_handler() callback once the entire request
+ *    body is available.
+ * 2. Inside data_handler(), the whole request body data is guaranteed to be
+ *    buffered, and this function can receive the whole request.
+ */
 ssize_t nxt_unit_request_read(nxt_unit_request_info_t *req, void *dst,
     size_t size);
 
+/* Read bytes until (and including) the next "\n" byte. */
 ssize_t nxt_unit_request_readline_size(nxt_unit_request_info_t *req,
     size_t max_size);
 
+/*
+ * Close the request.  This function must be called, or the request will hang.
+ *
+ * With NXT_UNIT_ERROR, if no parts of a response have been sent yet, Unit will
+ * send a default "503 Service Unavailable" response.
+ */
 void nxt_unit_request_done(nxt_unit_request_info_t *req, int rc);
 
 
 int nxt_unit_websocket_send(nxt_unit_request_info_t *req, uint8_t opcode,
     uint8_t last, const void *start, size_t size);

[callahad]

Let's split this into a patch that just corrects grammar / typos, which we can merge immediately. (These may have already been applied elsewhere).

For API documentation, there's an unresolved discussion around whether we want to document these things here in the header file, or in a separate location like in the NGINX Development Guide (which we don't have for Unit) or man pages.

This speaks to a broader subjective choice between trying to follow precedent from NGINX, or breaking with traditional patterns in hopes of being more accessible.

[callahad]

Quick consensus: happy to keep these comments in the headers. Need to verify that they're still accurate. @avahahn has volunteered to do so.

[avahahn]

Most of these look to remain accurate, I would like clarification on one of them.

[avahahn]

Does nxt_unit_response_write_nb still get repeatedly called? It appears to me that it is called once... Unless nxt_unit_response_write is being repeatedly called in which case we should clarify that in this doc string.