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..cd6ad36ae07635b24632ad4c85b37764f20aa53c
--- /dev/null
+++ b/net/docs/code-patterns.md
@@ -0,0 +1,244 @@
+# 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] [net_error_list.h]).

[asanka, 2015/09/21 20:31:01]

You can shorten this to [net_error_list.h][] where the link text will become the
link ID.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

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.
+
+Many functions also have variables (often named `result`) containing

[mmenke, 2015/09/21 19:46:28]

Maybe `result` or `rv`?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+such value; this is especially common in the [DoLoop](#DoLoop) pattern

[mmenke, 2015/09/21 19:46:28]

such value -> such a value

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+described below.
+
+## Sync/Async Return
+
+Many network stack routines may return synchronously or
+asynchronously. These functions generally return an int as described

[mmenke, 2015/09/21 19:46:28]

Should we mention void + taking a callback implies they return asynchronously?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Hmmm.  I'm inclined against, but I may not have a sense of how often this
happens.  The pattern I'm describing here is the sync/async combination, which I
can imagine would be confusing for people new to the stack.  If we were also
going to mention a pure async pattern, I'd want to put it in a separate heading,
but I wouldn't consider that worth doing unless it's fairly common, as I think
it's fairly clear what's going on for "void DoSomething(const Callback&
callback);"  So I'll leave this out, but please ping me back if you disagree and
I'll do a followup CL.

+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.

[mmenke, 2015/09/21 19:46:28]

and -> either...or ?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+* If the value is negative and != `net::ERR_IO_PENDING`, it is an error
+  code specifying a synchronous failing return.

[mmenke, 2015/09/21 19:46:28]

"failing return" sounds a little weird.  Maybe synchronous failing return ->
synchronous failure?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+* 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

[mmenke, 2015/09/21 19:46:28]

"An IOBuffer provided" -> "A reference to any provided IOBuffer"

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+  be written into or read from as required. Other pointers must be kept
+  alive manually until asynchronous completion is signaled.

[mmenke, 2015/09/21 19:46:28]

Other pointers?  You mean pointers for the called method to write into?  How
often do we actually do that?  May be too much of a special case to be worth
mentioning, and it's probably documented when we do it.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

On reflection, agreed; this isn't worth mentioning because it isn't a common
enough part of the pattern.  Done.

+  If a callback was provided, that callback will be called upon
+  completion with the return value; if a callback is not provided, it

[mmenke, 2015/09/21 19:46:28]

" a callback is not provided" -> "there is no callback argument"?  As-is, it's
ambiguous, since it could mean a NULL callback is passed in, which we often
don't allow.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+  usually means that some known callback mechanism will be employed.
+
+## DoLoop
+
+The DoLoop pattern is 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,

[mmenke, 2015/09/21 19:46:28]

nit:  single-threaded

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+if that function could block waiting for input. Generally initiation

[mmenke, 2015/09/21 19:46:28]

nit:  I'd suggest a comma after Generally.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+of a state machine is triggerred by some method invocation by a class

[mmenke, 2015/09/21 19:46:28]

nit:  triggered

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+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.

[mmenke, 2015/09/21 19:46:28]

"the state machine is reset" is not entirely accurate - they may set next_state_
to STATE_NONE and then wait to do the operation that logically follows the
previous operation, but I think calling that resetting the state machine very
confusing.

Maybe "paused" or "stopped"/"halted"?

Or if you're referring to objects that just invoke on callback and then
complete, those often aren't reusable, which is what resetting the state machine
means to me.  Or did you have something else in mind?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Hmmm.  This is tricky.  I'm making a distinction between the state machine (the
state that drives DoLoop(), generally only next_state_) and the rest of the
class (which can restart the state machine how it wants depending on what's
already happened).  Specifically, this is the distinction and issues I was
making in https://codereview.chromium.org/1315443003/#msg16; AFAIC, the state
machine itself is reset back to null upon completion of the dispatched
operation, whatever happens to the class as a whole.

I'll add a note that the state machine being reset doesn't mean that the class
is reset and that there may be some state carried over that will effect legality
or initial state for the next operation on the class, but I consider that
outside of the DoLoop pattern and importantly so.  I think I'll hold off for
your response on this before landing, as this seems a fairly important point.

+
+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`_<`STATE_NAME`>` (upper case separated by
+    underscores), and the routine is named Do`<`StateName`>` (CamelCase).
+    For example:
+
+         enum State {
+             STATE_NONE, 
+             STATE_INIT,
+             STATE_FOO,
+             STATE_FOO_COMPLETE,
+         };
+         int DoInit();
+         int DoFoo();
+         int DoFooComplete();

[asanka, 2015/09/21 20:31:01]

int DoFooComplete(int result);

esp since it's invoked this way below.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Ooops.  Thank you.  Done.

+
+*   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, the function saves the next state to a local
+    variable and resets to a default state (`STATE_NONE`),
+    and then calls the appropriate state handling based on the
+    original value of the next state. This looks like:
+
+           do {
+             State state = io_state_;
+             next_state_ = STATE_NONE;
+             switch (state) {
+               case STATE_INIT:
+                 result = DoInit();
+                 break;
+               ...
+
+    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 a given state may complete synchronously or asynchronously (for example,

[mmenke, 2015/09/21 19:46:28]

This is also true if a given state always completes asynchronously.  May not be
worth mentioning.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

I captured this by shortening the sentence to "If a given state may complete
asynchronously".  Done.

+    writing to an underlying transport socket), then there will often
+    be split 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. 
+    
+*   While the return value from each call is propagated through the loop
+    to the next state, it is expected that for most state transitions the
+    return value will be `net::OK`, and that an error return will also
+    set the state to `STATE_NONE` or fail to override the default
+    assignment to `STATE_DONE` to exit the loop and return that 
+    error to the caller. This is often asserted with a DCHECK, e.g.
+
+            case STATE_FOO:
+                DCHECK_EQ(result, OK);
+                result = DoFoo();
+                break;
+
+    The exception to this pattern is split states, where an IO
+    operation has been dispatched, and the second state is handling
+    the result. In that case, the second state's function takes the
+    result code:
+    
+            case STATE_FOO_COMPLETE:
+                result = DoFooComplete(result);
+                break;
+    
+*   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

[asanka, 2015/09/21 20:31:01]

value proper -> proper value

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+    call. In this case, `DoLoop()` will exit. This often occurs between
+    split states, as described above. 
+    
+*   The DoLoop mechanism  is generally invoked in response to a consumer

[mmenke, 2015/09/21 19:46:28]

nit:  Remove extra space

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+    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
+    explicitly 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

[mmenke, 2015/09/21 19:46:28]

ReadHeaders() to match DoLoop()?

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

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`. For example (from 
+    `HttpStreamParser`, abridged for clarity): 
+
+             int HttpStreamParser::ReadResponseHeaders(
+                 const CompletionCallback& callback) {
+               DCHECK(io_state_ == STATE_NONE || io_state_ == STATE_DONE);
+               DCHECK(callback_.is_null());
+               DCHECK(!callback.is_null());
+
+               int result = OK;
+               io_state_ = STATE_READ_HEADERS;
+
+               result = DoLoop(result);
+
+               if (result == ERR_IO_PENDING)
+                 callback_ = callback;
+
+               return result > 0 ? OK : result;
+             }
+
+    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()`.
+    Example: 
+
+             void HttpStreamParser::OnIOComplete(int result) {
+               result = DoLoop(result);
+
+               if (result != ERR_IO_PENDING && !callback_.is_null()) {
+                 CompletionCallback c = callback_;

[asanka, 2015/09/21 20:31:01]

base::ResetAndReturn() appears to be the canonical way to handle callback_.

[Randy Smith (Not in Mondays), 2015/09/22 01:38:21]

Done.

+                 callback_.Reset();
+                 c.Run(result);
+               }
+             }
+    
+*   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 generally have very little processing, primarily wrapping 
+`DoLoop()`. For `DoLoop()` entry this involves setting the `next_state_`
+variable, and possibly making copies of arguments into class members. For
+`DoLoop()` exit, it involves inspecting the return and passing it back to
+the caller, and in the asynchronous case, saving any passed completion callback
+for executing by a future subsidiary IO completion (see above example). 
+
+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)
+
+[net_error_list.h]: https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h#1