Writeup summary of common netstack coding patterns.

net/docs/code-patterns.md

Index: net/docs/code-patterns.md
diff --git a/net/docs/code-patterns.md b/net/docs/code-patterns.md
new file mode 100644
index 0000000000000000000000000000000000000000..1502ac5e3a87da96d072bd1a35d18a76256e3763
--- /dev/null
+++ b/net/docs/code-patterns.md
@@ -0,0 +1,171 @@
+# Chrome Network Stack Common Coding Patterns
+
+## Combined error and byte count into a single value
+
+At many places in the network stack, functions return a value that, if
+positive, indicate a count of bytes that the the function read or
+wrote, and if negative, indicates a network stack error code (see
+[net_error_list.h](https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h#1)).

[asanka, 2015/09/04 14:51:03]

Minor nit: I'd +1 Helen's suggestion to break this link. E.g.:

  ... indicates a network stack error code (see [net_error_list.h][]).
  Zero indiates either net::OK or zero bytes read (usually EOF)...


(and somewhere else)

  [net_error_list.h]: https://chro....

[Randy Smith (Not in Mondays), 2015/09/18 22:06:32]

Completely missed Helen's suggestion until you called it out--thank you.  Done.

+Zero indicates either net::OK or zero bytes read (usually EOF)
+depending on the context. This pattern is generally specified by
+an |int| return type.

[asanka, 2015/09/04 14:51:03]

Code is indicated using backticks. I.e. `int` instead of |int|. Here and
elsewhere.

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Done.

+
+Many functions also have variables (often named |result|) containing
+such value; this is especially common in the [DoLoop](#DoLoop) pattern
+described below.
+
+## Sync/Async Return
+
+Many network stack routines may return synchronously or
+asynchronously. These functions generally return an int as described
+above. There are three cases:
+
+* If the value is positive or zero, that indicates a synchronous
+  successful return, with a zero return value possibly indicating zero
+  bytes/EOF and possibly indicating net::OK, depending on context.

[asanka, 2015/09/04 14:51:03]

Nit: the problem with having to code format one identifier is that now you have
to code format other identifiers :P. I.e. `net::OK` instead of net::OK.

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Too true :-}.  Done (I hope).

+* If the value is negative and != `net::ERR_IO_PENDING`, it is an error
+  code specifying a synchronous failing return.
+* If the return value is the special value `net::ERR_IO_PENDING`, it
+  indicates that the routine will complete asynchronously. An IOBuffer
+  provided will be retained by the called entity until completion, to
+  be written into or read from as required. Other pointers must be kept
+  alive manually until asynchronous completion is signaled.
+  If a callback was provided, that callback will be called upon
+  completion with the return value; if a callback is not provided, it
+  usually means that some known callback mechanism will be employed.
+
+## DoLoop
+
+The DoLoop pattern is a pattern used in the network stack to construct

[asanka, 2015/09/04 14:51:03]

The DoLoop pattern is used in the network stack to ...

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Done.

+simple state machines. It is used for cases in which processing is

[asanka, 2015/09/04 14:51:03]

"simple" haha

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

I'm meta amused.  These state machines are *dirt* simple in the overall space of
state machines, at least the ones I have experience with.  Admittedly, I didn't
realize it until I started to write this documentation, but the DoLoop state
machines with all their hassle are the equivalent of a single function that
blocks for IO completion.   And wow can things get more complicated than that.

+basically single threaded and could be written in a single function,
+if that function could block waiting for input. Generally initiation
+of a state machine is triggerred by some method invocation by a class
+consumer, and that state machine is driven (possibly across
+asynchronous IO initiated by the class) until the operation requested
+by the method invocation completes, at which point the state machine
+is reset if completed and the consumer notified.
+
+Cases which do not fit into this single-threaded, single consumer
+operation model are generally adapted in some way to fit the model,
+either by multiple state machines (e.g. independent state machines for
+reading and writing, if each can be initiated while the other is
+outstanding) or by storing information across consumer invocations and
+returns that can be used to restart the state machine in the proper
+state. 
+
+Any class using this pattern will contain an enum listing all states
+of that machine, and define a function, `DoLoop()`, to drive that state
+machine. If a class has multiple state machines (as above) it will
+have multiple methods (e.g. `DoReadLoop()` and `DoWriteLoop()`) to drive
+those different machines.
+
+The characteristics of the DoLoop pattern are:
+
+*   Each state has a corresponding function which is called by `DoLoop()`
+    for handling when the state machine is in that state. Generally the
+    states are named STATE`_<`STATENAME`>` (upper case separated by

[asanka, 2015/09/04 14:51:03]

Minor nit: STATE_<STATE_NAME> or STATE_<NAME_OF_STATE> would be clearer.

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

http://www.guntheranderson.com/v/data/yourstat.htm :-}.  Done.

+    underscores), and the routine is named Do`<`StateName`>` (CamelCase).
+    Those functions both take and return values that are either

[asanka, 2015/09/04 14:51:03]

This deviates from what I've seen. Notably, individual states often require the
previous result to be net::OK, and don't typically take a result as a parameter.
This is good thing since it makes the states not overly dependent on their
respective incoming edges, nor are they burdened with having to deal with an
error condition introduced by an unknown previous state. E.g.

   int DoLoop(int result) {
     ...
     case STATE_FOO:
       DCHECK_EQ(result, OK);
       result = DoFoo();
       break;
     ...
   }

Split states, however, consist of a completion handler that *does* take a
result. This makes sense since the completion often depend on the result of the
async operation. It also eliminates the ambiguity of what the resulting int
means (byte count of what??). I.e.:

   ...
     case STATE_FOO:
       DCHECK_EQ(OK, result);
       result = DoFoo();
       break;

     case STATE_FOO_COMPLETE:
       result = DoFooComplete(result);
       break;
   ...

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Huh.  Right you are, at least based on the two canonical examples in this doc. 
I've updated the doc; further comments welcome.

+    net::Errors or the above combined error and byte count value.
+    
+*   If a given state may complete synchronously or asynchronously (for example,
+    writing to an underlying transport socket), then there will often
+    be pairs of related states, such as `STATE_WRITE` and
+    `STATE_WRITE_COMPLETE`. The first state is responsible for
+    starting/continuing the original operation, while the second state
+    is responsible for handling completion (e.g. success vs error,
+    complete vs. incomplete writes), and determining the next state to
+    transition to. 
+    
+*   Each state handling function has two basic responsibilities in
+    addition to state specific handling: Setting the data member
+    (named `next_state_` or something similar)
+    to specify the next state, and returning a net::Error (or combined
+    error and byte count, as above).
+    
+*   On each DoLoop iteration, it saves the next state to a local
+    variable and resets to the default/terminal state, and then calls
+    the appropriate state handling based on the original value of the
+    next state. This pattern is followed primarily to ensure that in
+    the event of a bug where the next state isn't set, the loop
+    terminates rather than loops infinitely. It's not a perfect
+    mitigation, but works well as a defensive measure.
+    
+*   If the return value from the state handling function is
+    `net::ERR_IO_PENDING`, that indicates that the function has arranged
+    for `DoLoop()` to be called at some point in the future, when further
+    progress can be made on the state transitions. The `next_state_` variable
+    will have been set to the value proper for handling that incoming
+    call. In this case, `DoLoop()` will exit.
+    
+*   A class state machine is generally invoked in response to a consumer
+    calling one of its methods. While the operation that method
+    requested is occuring, the state machine stays active, possibly
+    over multiple asynchronous operations and state transitions. When
+    that operation is complete, the state machine transitions to
+    `STATE_NONE` (by a `DoLoop()` callee not setting `next_state_`) or
+    `STATE_DONE` (by explicitly setting next_state_ to `STATE_DONE`

[asanka, 2015/09/04 14:51:03]

I don't see any consistent use of an explicit DONE state (not even in the
examples you cite below). Instead the pattern I see is to not set an explicit
next_state_ when a state handler wants to exit the state machine.

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Good point; oops.  Writing documentation CLs is a learning experience :-}.  I've
modified the text to be a bit more flexible and suggest what I think is the
common truth.

+    indicating that the operation is complete *and* the state machine is
+    not amenable to further driving). At this point the consumer is
+    notified of the completion of the operation (by synchronous return
+    or asynchronous callback).
+ 
+    Note that this implies that when `DoLoop()` returns, one of two
+    things will be true:
+ 
+    * The return value will be `net::ERR_IO_PENDING`, indicating that the
+      caller should take no action and instead wait for asynchronous
+      notification. 
+    * The state of the machine will be either `STATE_DONE` or `STATE_NONE`,
+      indicating that the operation that first initiated the `DoLoop()` has
+      completed. 
+ 
+    This invariant reflects and enforces the single-threaded (though
+    possibly asynchronous) nature of the driven state machine--the
+    machine is always executing one requested operation.
+    
+*   `DoLoop()` is called from two places: a) methods exposed to the consumer
+    for specific operations (e.g. |ReadHeaders|), and b) an IO completion

[asanka, 2015/09/04 14:51:03]

|o_O| -> `o_O`

[Randy Smith (Not in Mondays), 2015/09/18 22:06:33]

Done.

+    callbacks called asynchronously by spawned IO operations.
+
+    In the first case, the return value from `DoLoop()` is returned directly
+    to the caller; if the operation completed synchronously, that will
+    contain the operation result, and if it completed asynchronously, it
+    will be `net::ERR_IO_PENDING`.
+
+    In the second case, the IO completion callback will examine the
+    return value from `DoLoop()`. If it is `net::ERR_IO_PENDING`, no
+    further action will be taken, and the IO completion callback will be
+    called again at some future point. If it is not
+    `net::ERR_IO_PENDING`, that is a signal that the operation has
+    completed, and the IO completion callback will call the appropriate
+    consumer callback to notify the consumer that the operation has
+    completed. Note that it is important that this callback be done
+    from the IO completion callback and not from `DoLoop()` or a
+    `DoLoop()` callee, both to support the sync/async error return
+    (DoLoop and its callees don't know the difference) and to avoid
+    consumer callbacks deleting the object out from under `DoLoop()`.
+    
+*   The DoLoop pattern has no concept of different events arriving for
+    a single state; each state, if waiting, is waiting for one
+    particular event, and when `DoLoop()` is invoked when the machine is
+    in that state, it will handle that event. This reflects the
+    single-threaded model for operations spawned by the state machine.
+
+Public class methods have very little processing as possible,
+often simply making copies of arguments into data members, setting the
+`next_state_` variable to indicate the section of the state diagram to
+process, and calling `DoLoop()`.

[asanka, 2015/09/04 14:51:03]

Also, inspecting the return value of DoLoop and setting up the completion
callback. DoLoop() isn't aware of completion callbacks.

[Randy Smith (Not in Mondays), 2015/09/18 22:06:32]

Done.

+
+This idiom allows synchronous and asynchronous logic to be written in
+the same fashion; it's all just state transition handling. For mostly
+linear state diagrams, the handling code can be very easy to
+comprehend, as such code is usually written linearly (in different
+handling functions) in the order it's executed. 
+
+For examples of this idiom, see
+
+* [HttpStreamParser::DoLoop](https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stream_parser.cc&q=HttpStreamParser::DoLoop&sq=package:chromium).
+* [HttpNetworkTransaction::DoLoop](https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_network_transaction.cc&q=HttpNetworkTransaction::DoLoop&sq=package:chromium)
+