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

[xunjieli, 2015/08/31 19:29:32]

I find it easier to read if the reference links are added at the end so we don't
see long lines in the document.

e.g. at the end of the docment, we could do something like the following:

[net_error_list.h]:
https://chromium.googlesource.com/chromium/src/+blame/master/net/base/net_err...

[HttpStreamParser::DoLoop]:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

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

Ah, nice, I didn't know about that feature of markdown.  Thank you.

+Zero indicates either net::OK or zero bytes read (usually EOF)
+depending on the context.  This data union 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 usually indicating EOF.

[mmenke, 2015/08/28 16:24:57]

I'd go with usually -> possibly.

Basically if we're reading bytes directly, it generally indicates EOF (Though
some places we use ERR_CONNECTION_CLOSED for that - confusingly, I believe we
return different values on different OSes, at the lowest layer of the
stack...Erm..anyways...).  For anything else, it just means OK.

[Randy Smith (Not in Mondays), 2015/08/31 18:23:22]

Done.

+* 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.  Any buffer
+  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 (e.g. an asynchronous
+  return from
+  [URLRequestJob::ReadRawData](https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/url_request_job.h&q=ReadRawData&sq=package:chromium&l=309)

[mmenke, 2015/08/28 16:24:57]

I'd suggest avoiding URLRequestJob::ReadRawData - it's too weird and hairy. 
Maybe URLRequest::Read / URLRequestDelegate::OnReadCompleted?  Though that's
kinda hairy, too, given how we pass around the request's status.

[Randy Smith (Not in Mondays), 2015/08/31 18:23:22]

Hmmm.  I think based on this feedback I'll just remove the example pointer.  I
*think* it's better to not have an example than to have a confusing one, though
if you disagree, please let me know.

+  will be signalled by calling [URLRequestJob::NotifyReadComplete](https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/url_request_job.h&q=NotifyReadComplete&sq=package:chromium).
+
+## DoLoop
+
+The pattern usually used to construct state machines in the Chrome

[xunjieli, 2015/08/31 19:29:32]

usually [is] used?

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

I was using a shorthand language construct (You can think of it as intended to
be read as "DoLoop: The pattern usually used to ...") which was a bad idea and
which was modified in one of the other rounds of review.  Done.

+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
+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.
+* Each state handling function has two basic responsibilities in
+  addition to state specific handling: Setting the data member
+  (named |state_| or something similar)

[mmenke, 2015/08/28 16:24:57]

I'd say next_state_ is more common - when ERR_IO_PENDING is returned, in
particular, it's the state we're in when the callback is executed, not the
current state.

[Randy Smith (Not in Mondays), 2015/08/31 18:23:22]

Done.

+  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_.  
+* 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 return.

[mmenke, 2015/08/28 16:24:57]

will exit?

[Randy Smith (Not in Mondays), 2015/08/31 18:23:22]

Done.

+* If the return value from the state handling function is STATE_NONE
+  (indicating a failure to set the state_ variable) or STATE_DONE
+  (indicating that all state transitions have completed) DoLoop() will
+  also return.

[mmenke, 2015/08/28 16:24:57]

Suggest adding:

At this point, the return value will be returned (Either synchronously, or by
invoking a callback).
* When in STATE_NONE, the object is generally waiting for a particular call from
its consumer before re-entering its DoLoop.

[Randy Smith (Not in Mondays), 2015/08/31 18:23:22]

Done, though enough text is added that you should probably give it a read.

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

[xunjieli, 2015/08/31 19:29:32]

nit: s/differnet/different.

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

I think this one disappeared in one of the editing rounds.

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

[xunjieli, 2015/08/31 19:29:32]

s/much/must

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

Ditto.

+do the appropriate state transition.  
+
+For an example 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).

[mmenke, 2015/08/28 16:24:57]

Maybe add HttpNetworkTransaction as well?  I think those are the two
archetypical examples

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

Done.

+
+