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..d2ab5140b94bea0d70ea28b3edb78507b0012b60
--- /dev/null
+++ b/net/docs/code-patterns.md
@@ -0,0 +1,168 @@
+# 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)).
+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.
+
+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.
+* 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
+simple state machines. It is used for cases in which processing is
+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 of completed and the consumer notified.

[mmenke, 2015/09/02 16:41:17]

reset if completed?

[Randy Smith (Not in Mondays), 2015/09/02 20:25:54]

Yep, oops, done.

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

[mmenke, 2015/09/02 16:41:17]

My god!  Two spaces after a period.

[mmenke, 2015/09/02 16:43:05]

Worth noting:

1)  I'm trying to be silly, not criticizing anyone for pointing out
inconsistencies here.
2)  There are 6 other cases of this, currently (7 total).

[Randy Smith (Not in Mondays), 2015/09/02 20:25:54]

Indeed.  I will arrange to commit ritual hari-kari after this CL lands in my
shame :-}.  Done.

[Randy Smith (Not in Mondays), 2015/09/02 20:25:54]

No worries on my end; I've basically accepted that this should be our
documentation standard.  It's just not something I can manage to get very fussed
about.
Sigh.  I may have accepted it, but I haven't *executed* it well.  Thanks for
calling this out.  Done.

+    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`
+    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
+    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 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 events; each state, if

[mmenke, 2015/09/02 16:41:17]

This contradicts itself - you say no concept of events, then you say it's
waiting for an event.

Maybe you mean no concept of different types of events?  There are some
violations of that (HttpStreamTransaction has a couple different callbacks when
creating an HttpStream, for example), but it's generally true.

[Randy Smith (Not in Mondays), 2015/09/02 20:25:54]

So what I meant was that when the state machine is in a state and paused (i.e.
has returned ERR_IO_PENDING) it's waiting for a single event to happen, specific
to that state.  It's not that multiple events could arrive resulting in
different state transitions.  The IO (e.g.) will complete, and that completion
will be handled.  That handling may result in a fork based on some other test,
but that's not part of the state machine.
I couldn't find any instances of HttpStreamTransaction in the source.  I looked
at HttpNetworkTransaction, and it looks like it only has on IO Completion
callback, so I'm not sure what you mean.  Randomly guessing, I suspect that
different states may have different callbacks, but there's still only one
callback per state, but I'm not sure.  More specific pointer to the example?

[mmenke, 2015/09/02 20:31:29]

Sorry, I mean HttpNetworkTransaction.  Notice it's implementation of
HttpStreamRequest::Delegate.  I suppose they're all waiting for one operation to
progress, but they represent different "events", as I see it.  Perhaps we're
using different definitions of events.

+    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 should have as 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().
+
+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. If there can be
+multiple different events that complete outstanding IO, the framework
+doesn't handle that explicitly; the state handling code for the
+receiving state must explicitly distinguish between those events and
+do the appropriate state transition.
+
+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)
+