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..51c54a84f7496453acba5c83518cdada08896a57
--- /dev/null
+++ b/net/docs/code-patterns.md
@@ -0,0 +1,138 @@
+# 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.
+

[Ryan Sleevi, 2015/08/31 22:17:43]

Worth discussing the net::CompletionCallback that is tightly coupled to this
pattern?

[Randy Smith (Not in Mondays), 2015/09/02 02:32:28]

I detest net::CompletionCallback and consider it an anti-pattern.  Maybe you can
convince me otherwise?  The documentation for it is:

// A callback specialization that takes a single int parameter. Usually this is
// used to report a byte count or network error code.

which as far as I'm concerned doesn't count as a specification ("Usually"). 
It's used in different ways in different places, and as best I can tell was just
put together so that people wouldn't have to write base::Callback<void(int)>.  

If it was specified to always take the byte count/Error construct, and was used
that way, I'd be ok with that (though in an ideal world we'd have a typedef that
meant "byte count UNION error code", and it would take that--the compiler
wouldn't enforce it, but it would make intentions clear).  But it has 1800 uses,
I don't believe they're consistent, and in general I wish individual classes
would (fully) specify their own callbacks.

Ok, rant over.  And apologies--I'm sure it's not that bad.  But I don't like it.

[Ryan Sleevi, 2015/09/02 19:14:56]

Maybe, but I'm happy to save that discussion for another thread :) I consider
the alternatives quite an anti-pattern ('individual classes would (fully)
specify their own callbacks'), and I've seen the places in Chrome where this
pattern is fully embraced lead to all sorts of things I would call "grossness"
and hidden bugs.

But perhaps over coffee at the next net-stack all hands :)

+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 buffers must be kept

[Ryan Sleevi, 2015/08/31 22:17:43]

s/buffers/pointers/ (or objects)

[Randy Smith (Not in Mondays), 2015/09/02 02:32:29]

Done.

+  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 pattern usually used to construct state machines in the Chrome
+network stack is the DoLoop function. Any class that must drive a
+state machine will contain an enum listing all states of that machine,
+and define a function, |DoLoop|, to drive that state
+machine. Sometimes a single class may have multiple state machines,
+driven by multiple methods (e.g. DoFooLoop and DoBarLoop), and
+sometimes a larger state machine may be broken into smaller state
+machines, e.g. |DoHandshakeLoop|.
+
+The characteristics of the DoLoop pattern are:
+
+*    Each state has a corresponding function which is called by DoLoop

[Ryan Sleevi, 2015/08/31 22:17:43]

STYLE PEDANTRY: Google style is three spaces, not four, after a bullet :)

( https://github.com/google/styleguide/tree/gh-pages/docguide )

[Randy Smith (Not in Mondays), 2015/09/02 02:32:28]

Huh.  The markdown documentation that I found said that you wouldn't get the li
continuing on to the next paragraph without four, but it works.  Done.

+     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).
+     Those functions both take and return values that are either
+     net::Errors or the above combined error and byte count value.
+*    There are often pairs of related states, such as STATE_SEND_HEADERS
+     and STATE_SEND_HEADERS_COMPLETE.  The routine associated with the

[Ryan Sleevi, 2015/08/31 22:17:43]

pedantry: You introduced another double space :P [Yes, these stand out to me ;P)

[Randy Smith (Not in Mondays), 2015/09/02 02:32:29]

I *did* warn you :-}.  Done.
Good to know :-}.

+     second state handles completion processing (e.g. success vs. error)
+     and determining the next state to transition to (e.g. back to
+     STATE_SEND_HEADERS if the headers haven't actually all been sent).

[Ryan Sleevi, 2015/08/31 22:17:43]

I would suggest rewording this; we don't do FOO/FOO_COMPLETE pairs arbitrarily,
but rather as part of handling sync vs async.

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

[Randy Smith (Not in Mondays), 2015/09/02 02:32:28]

SGTM; this is exactly the kind of thing I'm relying on this review for.  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).
+*    DoLoop loops, saving next_state_ to a local variable and resetting
+     it to STATE_NONE, and then calling the appropriate state handling
+     based the original value of next_state_.

[Ryan Sleevi, 2015/08/31 22:17:43]

Suggestion: Add a sentence explaining why

*   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 good defensively.

[Randy Smith (Not in Mondays), 2015/09/02 02:32:28]

Thanks for the explanation; done.

+*    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 actively, 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. 
+  
+*    DoLoop is called from two places: a) methods exposed to the consumer
+     for specific operations (e.g. |ReadHeaders|), and b) 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 konw the difference) and to avoid consumer callbacks deleting

[Ryan Sleevi, 2015/08/31 22:17:43]

typo: know

[Randy Smith (Not in Mondays), 2015/09/02 02:32:28]

Done.

+     the object out from under DoLoop().
+
+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)
+