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..b79914b974b687fc124d36c58fe9cff592534eec
--- /dev/null
+++ b/net/docs/code-patterns.md
@@ -0,0 +1,92 @@
+# 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/+blame/master/net/base/net_error_list.h#1)).

[asanka, 2015/08/31 19:20:01]

Why is this a link to Git blame output?

[Randy Smith (Not in Mondays), 2015/08/31 21:55:06]

Oops; re-linked to the non-blame googlesource link.

+Zero indicates either net::OK or zero bytes read (usually EOF)
+depending on the context.  This data union is generally specified by

[Charlie Harrison, 2015/08/31 19:10:10]

"This data union" => "this pattern". The former feels as though in some way an
int is used to differentiate whether 0 is net::OK or an EOF.

[Ryan Sleevi, 2015/08/31 19:38:47]

Throughout this document, I note a use of two spaces after punctuation. On the
extremely pedantic grammar note, this is no longer considered correct
stylistically for virtually all styles (Strunk & White, Chicago, APA, MLA,
Turabian) that encouraged it :)

[mmenke, 2015/08/31 19:52:01]

I note that the reason I've always seen given for not considering it correct any
more is that "no one" uses fixed width fonts any more.  "no one" apparently
includes most software developers.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

*chuckle/sigh*  Yeah, the last time you brought this up, I acceded without being
convinced, and promptly forgot about it again.  It'll probably happen next time,
too.  Done.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

I don't have anywhere near a strong enough feeling on this to argue about it,
and I'm guessing that Ryan feels more strongly than you do, so I went with his
point despite basically agreeing with you.  If you feel strongly, please feel
free to hash the issue out with Ryan and I'll happily accept the result,
elsewise I've nuked the double spaces.  Sorry :-J.

[mmenke, 2015/08/31 21:59:20]

I don't feel strongly about it at all.  I just feel that it's one style issue
that's too silly to spend time arguing about.  And I default to double spaces,
because that's what I was taught in school...on a physical typewriter.

+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

[asanka, 2015/08/31 19:20:01]

`net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting?

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

I don't understand the comment?  Are you asking for quotes around the error
code?

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

Just that _FOO_ in Markdown indicates "emphasis on FOO", whereas `_FOO_` (note
backticks) indicates 'don't interpret the contents in the `` via markdown,
because it's already formatted/code formatted.

Mostly, making sure the "_" appears in the produced markdown as a literal _ and
not as an <em> or the like.

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

Done.

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

Ah, got it.  Done.

+  indicates that the routine will complete asynchronously.  Any buffer

[Ryan Sleevi, 2015/08/31 19:38:47]

Any IOBuffer?

I think this might use some expansion, to clarify that while an IOBuffer (which
supported ref-counting, even though it's not passed as scoped_refptr<>) may be
retained, other variables may not and need to be kept alive for the duration of
the call.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done.

+  provided will be retained by the called entity until completion, to
+  be written into or read from as required.  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.  The

[Ryan Sleevi, 2015/08/31 19:38:47]

It's worth noting that sometimes there are multiple state machines (e.g. in
response to event signalling), which may be signaled by DoFooLoop and DoBarLoop,
or that larger state machines may be broken into smaller state machines. (e.g.
DoHandshakeLoop)

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done.  

Can you give me a pointer to examples of these subpatterns?  I don't want to
include references to them in the docs, but I would like to look them over just
to get a sense as to how they work (and maybe include a note on that in the docs
if there's anything interesting about it).

[mmenke, 2015/08/31 21:59:20]

SpdySession has separate read and write loops.  I assume QUIC does as well.  If
our SSL sockets use DoLoops, they presumably do, too.  Not sure if there are any
other types of cases where we do it.

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

Yeah, DoReadLoop/DoWriteLoop/DoHandshakeLoop are the three examples that I often
think of.

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

Acknowledged.

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

I shifted the example names over to using this, but didn't reference the
specific examples.

+characteristics of this 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).
+  Those functions both take and return values that are either
+  net::Errors or the above combined error and byte count value.

[asanka, 2015/08/31 19:20:01]

Probably also mention pairs of state that are also common which look like:

 STATE_FOO
 STATE_FOO_COMPLETE (or STATE_FOO_COMPLETED).

Of course, you'd also need to explain what you'd put in STATE_FOO_COMPLETED
instead of another state.

Calling conventions for STATE_FOO_COMPLETE(D) is also often different in that
they take an explicit return value as a parameter.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done; take a look and see what you think.
I haven't seen that; in the cases I've looked at, *all* the DoLoop callees take
an explicit return value as a parameter.  It looks to me like part of the
pattern.  Example?

+* 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, initializes state_ to STATE_NONE, and calls the
+  appropriate state handling based the original value of state_.  

[Charlie Harrison, 2015/08/31 19:10:10]

What is "the original value of state_" if we've just initialized it?

[asanka, 2015/08/31 19:20:01]

s/state_/next_state_/ here an elsewhere.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:06]

Right, wrong way to put it (it saves the original value and resets state_). 
I've put in a bit more explanation.

+* 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 state_ variable
+  will have been set to the value proper for handling that incoming
+  call.  In this case, DoLoop() will exit.
+* If the return value from the state handling function is STATE_NONE
+  (indicating a failure to set the state_ variable) or STATE_DONE

[asanka, 2015/08/31 19:20:01]

Is it a failure to set the next state? I've mostly seen it used to indicate that
no other work needs to be done.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Partially a mis-typing on my part, partially my understanding growing as I get
comments on this CL.  I've reworded this bullet and the next one; let me know if
it doesn't satisfy your concern.

+  (indicating that all state transitions have completed) DoLoop() will
+  also return.  At this point, the return value from the operation being
+  driven by the state machine will be returned, either synchronously or
+  by invoking a callback.
+* When in STATE_NONE the object is generally waiting for a particular

[Charlie Harrison, 2015/08/31 19:10:10]

Is this an overloading of the point of STATE_NONE? The only time we set state_
to STATE_NONE above is if we fail to set  state_ (when would this happen?), or
when we initialize the state machine. Waiting for a call from a consumer seems
like a valid, non NONE state.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

On 2015/08/31 19:10:10, csharrison wrote:

Splitting response into implementation and philosophy.
So as I understand it, it's a valid choice for a callee of DoLoop to not set
state_, and that choice indicates that that callee of DoLoop believes that the
current iteration of the state machine drive is done (and usually that the
entity that originally started the state machine drive should be notified,
synchronously or by calback).  The idiom could have been constructed s.t. calls
from consumers were events that drove transitions on the state diagram, but I
think instead there are subsets of the state diagram that a given consumer call
initiates, and the call isn't considered completed until those subsets finish
whatever they're doing, at which point the state goes back to NONE.  

I've tried to capture this all in the doc; let me know if it makes sense.

I think this is relevant to David's point about single threaded versus
multi-threaded code; see his and my responses elsewhere in this CL.  That's not
yet in the doc, but will be after I'm confident I fully understand David's
point.

+  call from its consumer (e.g. a Read* call after some state machine
+  driven initialization) before re-entering its DoLoop.
+
+Public class methods should have as little processing as possible,
+often simply making copies of arguments into data members, setting the
+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 differnet events that complete outstanding IO, the framework

[asanka, 2015/08/31 19:20:01]

different

[Randy Smith (Not in Mondays), 2015/08/31 21:55:05]

Done.

+doesn't handle that explicitly; the state handling code for the
+receiving state much explicitly distinguish between those events and

[asanka, 2015/08/31 19:20:01]

must

[Randy Smith (Not in Mondays), 2015/08/31 21:55:06]

Done.

+do the appropriate state transition.  

[davidben, 2015/08/31 19:59:14]

I think I might phrase this more strongly. It's not just multiple different
events. It's that your single thread of execution may only wait on exactly one
thing. If you ever fire two asynchronous things in parallel, you have to
synchronize things. If you get events coming in from outside, you have to make
sure not to transition the state machine unless it has gotten to the point that
you're waiting on it. Otherwise you, e.g., set some boolean somewhere.

I think the single thread of execution analogy is actually pretty key. It's a
fairly routine transformation from:

int Foo::ConnectSync() {
  int ret = SendRequestSync();
  if (ret != OK)
    return ret;

  int ret = ReceiveResponseSync();
  if (ret != OK)
    return ret;

  return OK;
}

to:

int Foo::DoSendRequest() {
  next_state_ = STATE_SEND_REQUEST_COMPLETE;
  return SendRequest(io_callback_);
}

int Foo::DoSendRequestComplete(int ret) {
  if (ret != OK)
    return ret;

  // Optional: No reason why these two can't be folded together.
  next_state_ = STATE_RECEIVE_RESPONSE;
  return OK;
}

int Foo::DoReceiveResponse() {
  next_state_ = STATE_RECEIVED_RESPONSE_COMPLETE;
  return ReceiveResponse(io_callback_);
}

int Foo::DoReceiveResponseComplete(int ret) {
  if (ret != OK)
    return ret;

  return OK;
}

Embellished with random synchronous-only work and temporaries being saved in Foo
and whatnot as you see fit. And, of course, branching requires additional states
that may not be completely linear, etc. As long as it's all single-threaded,
you're happy.


The relevant point between multiple events vs single-threaded is that, say
instead of ReceiveResponse's completion being triggered by the callback, Foo is
a BlahBlahDelegate and has an OnResponseReceived method. *Now* you potentially
have a problem. If it's possible for OnResponseReceived to trigger not perfectly
paired with a ReceiveResponse call (say, you're SPDY, and you might get pushed a
response early), then this pattern falls over. OnResponseReceived needs to have
extra state to transform it into something that can be paired perfectly with
something that pauses. Otherwise multiple kinds of events aren't really a
problem.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:06]

So a) thank you, the whole reason I personally wrote this CL was to scare out
things about DoLoop that I didn't understand, and you've just nailed one such,
but b) I *think* the single threaded restriction only occurs in terms of
consumer entry points and response to those entry points.  I.e. if you call
ReceiveResponse, it can, synchronously or asynchronously, drive the DoLoop to
its heart's content, but only until the point where it signals back to the
caller that it actually *has* received the response, at which point the state
variable is set to STATE_NONE and anything that'll affect future state
transitions had better be in side variables unrelated to the main state machine.
 However, if, during the execution of the section of the section of the state
machine that handles receiving responses, it's fine to have multi-threaded.  You
spawn out (DoSpawn/STATE_SPAWN) two different events whose results may come back
into either order, and in the DoSpawnComplete function you do whichever state
transition the result indicates you do.  Any states that may wait and get
sparked through a completion callback until the other one comes in will need to
handle the other result, but that's semi-standard state machine stuff.

If that's what you meant, I'll add it to the doc and feel a solid glow of
satisfaction about it.  But I want to confirm I've really groked your point
first :-} :-|.

[davidben, 2015/08/31 22:19:35]

Hrm. I'm not sure I follow what you're saying. Grab me tomorrow perhaps?

+

[Ryan Sleevi, 2015/08/31 19:38:47]

One area I've seen frequently trip people up in reviews is how it comes to event
signalling.

Events are only signaled after the return of the DoLoop, by copying all internal
state to the stack, and signalling. This is to ensure that, in response to an
event, any necessary teardown can occur.

For example, I think it would be good to discuss the state machine mechanics in
terms of linear, synchronous state (which I would argue this document does
fairly close to well enough), but then also talk about handing asynchronous
events from the perspective of the state machine (that is, the OnIOComplete
entry point which signals DoLoop), and also the signaling of events to callers
after a state machine indicated an asynchronous result would be provided (line
28)

In particular, the memoization of how the DoLoop() began, so that it's clear how
to signal back that processing is over. Here, the sockets are perhaps a better
example of this than the HTTP cases.

[Randy Smith (Not in Mondays), 2015/08/31 21:55:06]

Thank you; as I noted to David, one of my agendas in doing this doc was to learn
things about DoLoop I didn't know, and this is one such thing.  I've attempted
to capture this point in text in the doc; let me know what you think.

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