Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320933003:
Writeup summary of common netstack coding patterns. (Closed)
|
Created:
5 years, 3 months ago by Randy Smith (Not in Mondays) Modified:
5 years, 3 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWriteup summary of common netstack coding patterns.
BUG=
Committed: https://crrev.com/6c8f81939621e202bb3b4d8806382e9987bc7392
Cr-Commit-Position: refs/heads/master@{#350173}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Incorporated Matt's comments. #
Total comments: 41
Patch Set 3 : Incorporated many comments. #
Total comments: 15
Patch Set 4 : Incorporate comments, discuss single threading in various forms. #
Total comments: 9
Patch Set 5 : Fixed typos, double spaces after periods, and clarified single/multiple events. #Patch Set 6 : Cleanup pass. #
Total comments: 20
Patch Set 7 : Incorporated all comments, reflowed, and added examples. #
Total comments: 38
Patch Set 8 : Incorporated comments. #
Total comments: 2
Patch Set 9 : Resolve "resetting state machine" wording. #Messages
Total messages: 49 (10 generated)
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
Matt: Could you give me a reality check as to whether you think this is worth doing? If so, I'll pull in a couple of reviewers (you+sleevi for correctness and completeness, xunjieli for comprehensibility) and try to polish it into something land-able.
On 2015/08/27 20:47:42, rdsmith wrote: > Matt: Could you give me a reality check as to whether you think this is worth > doing? If so, I'll pull in a couple of reviewers (you+sleevi for correctness > and completeness, xunjieli for comprehensibility) and try to polish it into > something land-able. I'm honestly not sure whether or not this is useful to people new to the network stack. Maybe bring in csharrison, and see if he finds it informative?
rdsmith@chromium.org changed reviewers: + csharrison@chromium.org
Fair enough. Charles, do you have an opinion? I'll note that I don't personally feel like I'm yet solidly on top of the DoLoop pattern, and I'm hoping that I'll be able to refine and expand on it with the help of my reviewers (and I've been here five years, so presumably what's at the edge of my understanding has some use for new people). That's most of why I did this, I just decided I needed to explain byte count+Error & sync/async before I dove into DoLoop.
On 2015/08/28 15:48:26, rdsmith wrote: > Fair enough. Charles, do you have an opinion? > > I'll note that I don't personally feel like I'm yet solidly on top of the DoLoop > pattern, and I'm hoping that I'll be able to refine and expand on it with the > help of my reviewers (and I've been here five years, so presumably what's at the > edge of my understanding has some use for new people). That's most of why I did > this, I just decided I needed to explain byte count+Error & sync/async before I > dove into DoLoop. I think it's informative, at least from a cursory read. I haven't touched a lot of networking internals yet but I have been confused by the DoLoop construct. The sync/async stuff about net::ERR_IO_PENDING is good to know as it's a special case and not really an error.
Some suggestions https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:23: successful return, with a zero return value usually indicating EOF. 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:34: [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) 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:53: (named |state_| or something similar) 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:63: call. In this case, DoLoop() will return. will exit? https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:67: also return. 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:84: 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). Maybe add HttpNetworkTransaction as well? I think those are the two archetypical examples
Incorporated comments; PTAL? Charles, since you're already involved, would you be willing to do a thorough review for comprehensibility and completeness (i.e. can you understand it and does it answer all the questions you have about the coding patterns?). I'm also going to let folks on the state machine thread know about it, in case anyone feels like doing a drive-by. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:23: successful return, with a zero return value usually indicating EOF. On 2015/08/28 16:24:57, mmenke wrote: > 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. Done. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:34: [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) On 2015/08/28 16:24:57, mmenke wrote: > 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. 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:53: (named |state_| or something similar) On 2015/08/28 16:24:57, mmenke wrote: > 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. Done. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:63: call. In this case, DoLoop() will return. On 2015/08/28 16:24:57, mmenke wrote: > will exit? Done. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:67: also return. On 2015/08/28 16:24:57, mmenke wrote: > 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. Done, though enough text is added that you should probably give it a read.
On 2015/08/31 18:23:22, rdsmith wrote: > Incorporated comments; PTAL? > > Charles, since you're already involved, would you be willing to do a thorough > review for comprehensibility and completeness (i.e. can you understand it and > does it answer all the questions you have about the coding patterns?). > > I'm also going to let folks on the state machine thread know about it, in case > anyone feels like doing a drive-by. > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md > File net/docs/code-patterns.md (right): > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... > net/docs/code-patterns.md:23: successful return, with a zero return value > usually indicating EOF. > On 2015/08/28 16:24:57, mmenke wrote: > > 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. > > Done. > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... > net/docs/code-patterns.md:34: > [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) > On 2015/08/28 16:24:57, mmenke wrote: > > 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. > > 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. > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... > net/docs/code-patterns.md:53: (named |state_| or something similar) > On 2015/08/28 16:24:57, mmenke wrote: > > 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. > > Done. > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... > net/docs/code-patterns.md:63: call. In this case, DoLoop() will return. > On 2015/08/28 16:24:57, mmenke wrote: > > will exit? > > Done. > > https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... > net/docs/code-patterns.md:67: also return. > On 2015/08/28 16:24:57, mmenke wrote: > > 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. > > Done, though enough text is added that you should probably give it a read. *chuckle* All of the above, and now I've uploaded the new PS too. Thanks to Charles for catching that.
Nice explanations, just a few nitpicks. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by "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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:56: appropriate state handling based the original value of state_. What is "the original value of state_" if we've just initialized it? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:69: * When in STATE_NONE the object is generally waiting for a particular 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.
asanka@chromium.org changed reviewers: + asanka@chromium.org
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+blame/master/net/base/net_error_list.h#1)). Why is this a link to Git blame output? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:28: * If the return value is the special value net::ERR_IO_PENDING, it `net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:49: net::Errors or the above combined error and byte count value. 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:56: appropriate state handling based the original value of state_. s/state_/next_state_/ here an elsewhere. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:64: (indicating a failure to set the state_ variable) or STATE_DONE 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:83: multiple differnet events that complete outstanding IO, the framework different https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:85: receiving state much explicitly distinguish between those events and must
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
driving by https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+blame/master/net/base/net_error_list.h#1)). 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... https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:39: The pattern usually used to construct state machines in the Chrome usually [is] used? https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:79: multiple differnet events that complete outstanding IO, the framework nit: s/differnet/different. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:81: receiving state much explicitly distinguish between those events and s/much/must
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by 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 :) https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:29: indicates that the routine will complete asynchronously. Any buffer 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The 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) https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:87: 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.
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by On 2015/08/31 19:38:47, Ryan Sleevi wrote: > 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 :) 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.
davidben@chromium.org changed reviewers: + davidben@chromium.org
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:86: do the appropriate state transition. 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.
(Oh, it seems a lot of people said stuff while I was typing. It's possible I'm being redundant with everyone else.)
Comments incorporated. Specific thanks to David and Ryan, who've taught me things I didn't know about DoLoop (a hidden agenda of doing this CL :-}). PTAL? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+blame/master/net/base/net_error_list.h#1)). On 2015/08/31 19:20:01, asanka (On leave) wrote: > Why is this a link to Git blame output? Oops; re-linked to the non-blame googlesource link. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by On 2015/08/31 19:10:10, csharrison wrote: > "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. Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by On 2015/08/31 19:38:47, Ryan Sleevi wrote: > 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 :) *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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by On 2015/08/31 19:52:01, mmenke wrote: > On 2015/08/31 19:38:47, Ryan Sleevi wrote: > > 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 :) > > 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. 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:28: * If the return value is the special value net::ERR_IO_PENDING, it On 2015/08/31 19:20:01, asanka (On leave) wrote: > `net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting? I don't understand the comment? Are you asking for quotes around the error code? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:29: indicates that the routine will complete asynchronously. Any buffer On 2015/08/31 19:38:47, Ryan Sleevi wrote: > 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. Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The On 2015/08/31 19:38:47, Ryan Sleevi wrote: > 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) 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). https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:49: net::Errors or the above combined error and byte count value. On 2015/08/31 19:20:01, asanka (On leave) wrote: > 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. Done; take a look and see what you think. > Calling conventions for STATE_FOO_COMPLETE(D) is also often different in that > they take an explicit return value as a parameter. 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? https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:56: appropriate state handling based the original value of state_. On 2015/08/31 19:10:10, csharrison wrote: > What is "the original value of state_" if we've just initialized it? Right, wrong way to put it (it saves the original value and resets state_). I've put in a bit more explanation. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:56: appropriate state handling based the original value of state_. On 2015/08/31 19:20:01, asanka (On leave) wrote: > s/state_/next_state_/ here an elsewhere. Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:64: (indicating a failure to set the state_ variable) or STATE_DONE On 2015/08/31 19:20:01, asanka (On leave) wrote: > 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. 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:69: * When in STATE_NONE the object is generally waiting for a particular On 2015/08/31 19:10:10, csharrison wrote: Splitting response into implementation and philosophy. > 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. 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:83: multiple differnet events that complete outstanding IO, the framework On 2015/08/31 19:20:01, asanka (On leave) wrote: > different Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:85: receiving state much explicitly distinguish between those events and On 2015/08/31 19:20:01, asanka (On leave) wrote: > must Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:86: do the appropriate state transition. On 2015/08/31 19:59:14, David Benjamin wrote: > 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. 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 :-} :-|. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:87: On 2015/08/31 19:38:47, Ryan Sleevi wrote: > 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. 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.
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:10: depending on the context. This data union is generally specified by On 2015/08/31 21:55:05, rdsmith wrote: > On 2015/08/31 19:52:01, mmenke wrote: > > On 2015/08/31 19:38:47, Ryan Sleevi wrote: > > > 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 :) > > > > 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. > > 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. 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The On 2015/08/31 21:55:05, rdsmith wrote: > On 2015/08/31 19:38:47, Ryan Sleevi wrote: > > 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) > > 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). 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.
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:28: * If the return value is the special value net::ERR_IO_PENDING, it On 2015/08/31 21:55:05, rdsmith wrote: > On 2015/08/31 19:20:01, asanka (On leave) wrote: > > `net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting? > > I don't understand the comment? Are you asking for quotes around the error > code? 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. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The On 2015/08/31 21:59:20, mmenke wrote: > 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. Yeah, DoReadLoop/DoWriteLoop/DoHandshakeLoop are the three examples that I often think of. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:12: Worth discussing the net::CompletionCallback that is tightly coupled to this pattern? https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:31: be written into or read from as required. Other buffers must be kept s/buffers/pointers/ (or objects) https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:50: * Each state has a corresponding function which is called by DoLoop STYLE PEDANTRY: Google style is three spaces, not four, after a bullet :) ( https://github.com/google/styleguide/tree/gh-pages/docguide ) https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:57: and STATE_SEND_HEADERS_COMPLETE. The routine associated with the pedantry: You introduced another double space :P [Yes, these stand out to me ;P) https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:60: STATE_SEND_HEADERS if the headers haven't actually all been sent). 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. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:68: based the original value of next_state_. 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. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:116: don't konw the difference) and to avoid consumer callbacks deleting typo: know
https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:86: do the appropriate state transition. On 2015/08/31 21:55:06, rdsmith wrote: > On 2015/08/31 19:59:14, David Benjamin wrote: > > 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. > > 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 :-} :-|. Hrm. I'm not sure I follow what you're saying. Grab me tomorrow perhaps?
Another round of comments incorporated; PTAL? Matt, Ryan: Based on an extended conversation with David, I've included some more text on the basically single-threaded nature of DoLoop; AINUI, it is *not* intended as a general state machine idiom (which makes a lot of code I've read make a lot more sense). I've tried to capture the what and the why of this in the first couple of paragraphs, but I'm relying on you folks to let me know if I've gone overboard. This CL is being a learning experience for me. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:28: * If the return value is the special value net::ERR_IO_PENDING, it On 2015/08/31 22:17:43, Ryan Sleevi wrote: > On 2015/08/31 21:55:05, rdsmith wrote: > > On 2015/08/31 19:20:01, asanka (On leave) wrote: > > > `net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting? > > > > I don't understand the comment? Are you asking for quotes around the error > > code? > > 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. Ah, got it. Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:28: * If the return value is the special value net::ERR_IO_PENDING, it On 2015/08/31 19:20:01, asanka (On leave) wrote: > `net::ERR_IO_PENDING` ? Would the underscores be interpreted as formatting? Done. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The On 2015/08/31 21:59:20, mmenke wrote: > On 2015/08/31 21:55:05, rdsmith wrote: > > On 2015/08/31 19:38:47, Ryan Sleevi wrote: > > > 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) > > > > 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). > > 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. I shifted the example names over to using this, but didn't reference the specific examples. https://codereview.chromium.org/1320933003/diff/20001/net/docs/code-patterns.... net/docs/code-patterns.md:41: and define a function, |DoLoop|, to drive that state machine. The On 2015/08/31 22:17:43, Ryan Sleevi wrote: > On 2015/08/31 21:59:20, mmenke wrote: > > 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. > > Yeah, DoReadLoop/DoWriteLoop/DoHandshakeLoop are the three examples that I often > think of. Acknowledged. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:12: On 2015/08/31 22:17:43, Ryan Sleevi wrote: > Worth discussing the net::CompletionCallback that is tightly coupled to this > pattern? 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. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:31: be written into or read from as required. Other buffers must be kept On 2015/08/31 22:17:43, Ryan Sleevi wrote: > s/buffers/pointers/ (or objects) Done. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:50: * Each state has a corresponding function which is called by DoLoop On 2015/08/31 22:17:43, Ryan Sleevi wrote: > STYLE PEDANTRY: Google style is three spaces, not four, after a bullet :) > > ( https://github.com/google/styleguide/tree/gh-pages/docguide ) 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. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:57: and STATE_SEND_HEADERS_COMPLETE. The routine associated with the On 2015/08/31 22:17:43, Ryan Sleevi wrote: > pedantry: You introduced another double space :P I *did* warn you :-}. Done. > [Yes, these stand out to me ;P) Good to know :-}. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:60: STATE_SEND_HEADERS if the headers haven't actually all been sent). On 2015/08/31 22:17:43, Ryan Sleevi wrote: > 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. SGTM; this is exactly the kind of thing I'm relying on this review for. Done. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:68: based the original value of next_state_. On 2015/08/31 22:17:43, Ryan Sleevi wrote: > 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. Thanks for the explanation; done. https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:116: don't konw the difference) and to avoid consumer callbacks deleting On 2015/08/31 22:17:43, Ryan Sleevi wrote: > typo: know Done.
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:47: is reset of completed and the consumer notified. reset if completed? https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:98: calling one of its methods. While the operation that method My god! Two spaces after a period. https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of events; each state, if 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.
https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:98: calling one of its methods. While the operation that method On 2015/09/02 16:41:17, mmenke wrote: > My god! Two spaces after a period. 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).
After re-reading all of this, mod the two space issues/trailing whitespace, it LGTM :) https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/40001/net/docs/code-patterns.... net/docs/code-patterns.md:12: On 2015/09/02 02:32:28, rdsmith wrote: > I detest net::CompletionCallback and consider it an anti-pattern. Maybe you can > convince me otherwise? 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 :)
Patchset #6 (id:100001) has been deleted
Thanks, Ryan! And yes, coffee at the next all-hands. Or maybe alcohol; coffee may be more psychoactive than I can deal with :-}. Matt: Comments incorporated, PTAL? Helen and/or Charles: Could you also take a look? I'd like one of you (or both if you're both interested) to get to the point of being happy to stamp this, as I think you're closer to the intended audience that Matt/Ryan. A note on patchsets: PS5 is my response to Matt's comments. PS6 is my own cleanup, and mostly involved regularizing references to DoLoop() (outside of || but in quoted font when it's referring to code, just a word when it refers to the pattern) and other making sure other code references were quoted. https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:47: is reset of completed and the consumer notified. On 2015/09/02 16:41:17, mmenke wrote: > reset if completed? Yep, oops, done. https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:98: calling one of its methods. While the operation that method On 2015/09/02 16:41:17, mmenke wrote: > My god! Two spaces after a period. Indeed. I will arrange to commit ritual hari-kari after this CL lands in my shame :-}. Done. https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:98: calling one of its methods. While the operation that method On 2015/09/02 16:43:05, mmenke wrote: > On 2015/09/02 16:41:17, mmenke wrote: > > My god! Two spaces after a period. > > Worth noting: > > 1) I'm trying to be silly, not criticizing anyone for pointing out > inconsistencies here. 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. > 2) There are 6 other cases of this, currently (7 total). Sigh. I may have accepted it, but I haven't *executed* it well. Thanks for calling this out. Done. https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of events; each state, if On 2015/09/02 16:41:17, mmenke wrote: > 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? 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. > There are some > violations of that (HttpStreamTransaction has a couple different callbacks when > creating an HttpStream, for example), but it's generally true. 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?
https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of events; each state, if On 2015/09/02 20:25:54, rdsmith wrote: > On 2015/09/02 16:41:17, mmenke wrote: > > 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? > > 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. > > > There are some > > violations of that (HttpStreamTransaction has a couple different callbacks > when > > creating an HttpStream, for example), but it's generally true. > > 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? 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.
On 2015/09/02 20:31:29, mmenke wrote: > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md > File net/docs/code-patterns.md (right): > > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... > net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of events; > each state, if > On 2015/09/02 20:25:54, rdsmith wrote: > > On 2015/09/02 16:41:17, mmenke wrote: > > > 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? > > > > 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. > > > > > There are some > > > violations of that (HttpStreamTransaction has a couple different callbacks > > when > > > creating an HttpStream, for example), but it's generally true. > > > > 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? > > 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. Ah, good point. What I'm trying to capture is that the *idiom* doesn't have any support for different incoming events as a top level concept; everything gets funneled down to DoLoop(), and if there's some distinction as to what prompted the call to DoLoop(), it needs to be saved in some set of variables that isn't part of the base idiom. To put it differently, there's an enum for states, but no enum, or dispatch mechanism, for different events. Any thoughts as to how I could express that better than I have?
(Just another quick drive-by comment; I mostly just skimmed the new version, not looked at it very closely. It might be valuable to have a simple example or two, rather than spell out all the rules in prose. Some simple-ish synchronous function and its corresponding DoLoop transformation. Might help with the "single-threaded" notion.)
On 2015/09/02 21:08:16, rdsmith wrote: > On 2015/09/02 20:31:29, mmenke wrote: > > > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md > > File net/docs/code-patterns.md (right): > > > > > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... > > net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of > events; > > each state, if > > On 2015/09/02 20:25:54, rdsmith wrote: > > > On 2015/09/02 16:41:17, mmenke wrote: > > > > 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? > > > > > > 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. > > > > > > > There are some > > > > violations of that (HttpStreamTransaction has a couple different callbacks > > > when > > > > creating an HttpStream, for example), but it's generally true. > > > > > > 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? > > > > 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. > > Ah, good point. > > What I'm trying to capture is that the *idiom* doesn't have any support for > different incoming events as a top level concept; everything gets funneled down > to DoLoop(), and if there's some distinction as to what prompted the call to > DoLoop(), it needs to be saved in some set of variables that isn't part of the > base idiom. To put it differently, there's an enum for states, but no enum, or > dispatch mechanism, for different events. > > Any thoughts as to how I could express that better than I have? Maybe just: The DoLoop pattern has no concept of different events arriving for -> The DoLoop pattern typically has no concept... Or perhaps: "The DoLoop pattern typically involves waiting for a single event at a time. Either it's waiting for the last operation it initiated to complete, or it recently invoked the consumer's callback, and is waiting for the consumer to call a new method."
On 2015/09/02 21:53:13, mmenke wrote: > On 2015/09/02 21:08:16, rdsmith wrote: > > On 2015/09/02 20:31:29, mmenke wrote: > > > > > > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.md > > > File net/docs/code-patterns.md (right): > > > > > > > > > https://codereview.chromium.org/1320933003/diff/60001/net/docs/code-patterns.... > > > net/docs/code-patterns.md:143: * The DoLoop pattern has no concept of > > events; > > > each state, if > > > On 2015/09/02 20:25:54, rdsmith wrote: > > > > On 2015/09/02 16:41:17, mmenke wrote: > > > > > 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? > > > > > > > > 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. > > > > > > > > > There are some > > > > > violations of that (HttpStreamTransaction has a couple different > callbacks > > > > when > > > > > creating an HttpStream, for example), but it's generally true. > > > > > > > > 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? > > > > > > 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. > > > > Ah, good point. > > > > What I'm trying to capture is that the *idiom* doesn't have any support for > > different incoming events as a top level concept; everything gets funneled > down > > to DoLoop(), and if there's some distinction as to what prompted the call to > > DoLoop(), it needs to be saved in some set of variables that isn't part of the > > base idiom. To put it differently, there's an enum for states, but no enum, > or > > dispatch mechanism, for different events. > > > > Any thoughts as to how I could express that better than I have? > > Maybe just: > > The DoLoop pattern has no concept of different events arriving for -> > The DoLoop pattern typically has no concept... > > Or perhaps: > > "The DoLoop pattern typically involves waiting for a single event at a time. > Either it's waiting for the last operation it initiated to complete, or it > recently invoked the consumer's callback, and is waiting for the consumer to > call a new method." (Could add something emphasizing it generally can't be interrupted by other events while it's waiting, other than deletion / cancellation)
https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h#1)). Minor nit: I'd +1 Helen's suggestion to break this link. E.g.: ... indicates a network stack error code (see [net_error_list.h][]). Zero indiates either net::OK or zero bytes read (usually EOF)... (and somewhere else) [net_error_list.h]: https://chro.... https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:11: an |int| return type. Code is indicated using backticks. I.e. `int` instead of |int|. Here and elsewhere. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:25: bytes/EOF and possibly indicating net::OK, depending on context. Nit: the problem with having to code format one identifier is that now you have to code format other identifiers :P. I.e. `net::OK` instead of net::OK. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:39: The DoLoop pattern is a pattern used in the network stack to construct The DoLoop pattern is used in the network stack to ... https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:40: simple state machines. It is used for cases in which processing is "simple" haha https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:67: states are named STATE`_<`STATENAME`>` (upper case separated by Minor nit: STATE_<STATE_NAME> or STATE_<NAME_OF_STATE> would be clearer. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:69: Those functions both take and return values that are either This deviates from what I've seen. Notably, individual states often require the previous result to be net::OK, and don't typically take a result as a parameter. This is good thing since it makes the states not overly dependent on their respective incoming edges, nor are they burdened with having to deal with an error condition introduced by an unknown previous state. E.g. int DoLoop(int result) { ... case STATE_FOO: DCHECK_EQ(result, OK); result = DoFoo(); break; ... } Split states, however, consist of a completion handler that *does* take a result. This makes sense since the completion often depend on the result of the async operation. It also eliminates the ambiguity of what the resulting int means (byte count of what??). I.e.: ... case STATE_FOO: DCHECK_EQ(OK, result); result = DoFoo(); break; case STATE_FOO_COMPLETE: result = DoFooComplete(result); break; ... https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:108: `STATE_DONE` (by explicitly setting next_state_ to `STATE_DONE` I don't see any consistent use of an explicit DONE state (not even in the examples you cite below). Instead the pattern I see is to not set an explicit next_state_ when a state handler wants to exit the state machine. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:129: for specific operations (e.g. |ReadHeaders|), and b) an IO completion |o_O| -> `o_O` https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:159: process, and calling `DoLoop()`. Also, inspecting the return value of DoLoop and setting up the completion callback. DoLoop() isn't aware of completion callbacks.
On 2015/09/04 14:51:04, asanka (On leave) wrote: > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns.md > File net/docs/code-patterns.md (right): > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:8: > [net_error_list.h](https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h#1)). > Minor nit: I'd +1 Helen's suggestion to break this link. E.g.: > > ... indicates a network stack error code (see [net_error_list.h][]). > Zero indiates either net::OK or zero bytes read (usually EOF)... > > > (and somewhere else) > > [net_error_list.h]: https://chro.... > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:11: an |int| return type. > Code is indicated using backticks. I.e. `int` instead of |int|. Here and > elsewhere. > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:25: bytes/EOF and possibly indicating net::OK, > depending on context. > Nit: the problem with having to code format one identifier is that now you have > to code format other identifiers :P. I.e. `net::OK` instead of net::OK. > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:39: The DoLoop pattern is a pattern used in the > network stack to construct > The DoLoop pattern is used in the network stack to ... > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:40: simple state machines. It is used for cases in > which processing is > "simple" haha > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:67: states are named STATE`_<`STATENAME`>` (upper case > separated by > Minor nit: STATE_<STATE_NAME> or STATE_<NAME_OF_STATE> would be clearer. > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:69: Those functions both take and return values that > are either > This deviates from what I've seen. Notably, individual states often require the > previous result to be net::OK, and don't typically take a result as a parameter. > This is good thing since it makes the states not overly dependent on their > respective incoming edges, nor are they burdened with having to deal with an > error condition introduced by an unknown previous state. E.g. > > int DoLoop(int result) { > ... > case STATE_FOO: > DCHECK_EQ(result, OK); > result = DoFoo(); > break; > ... > } > > Split states, however, consist of a completion handler that *does* take a > result. This makes sense since the completion often depend on the result of the > async operation. It also eliminates the ambiguity of what the resulting int > means (byte count of what??). I.e.: > > ... > case STATE_FOO: > DCHECK_EQ(OK, result); > result = DoFoo(); > break; > > case STATE_FOO_COMPLETE: > result = DoFooComplete(result); > break; > ... > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:108: `STATE_DONE` (by explicitly setting next_state_ > to `STATE_DONE` > I don't see any consistent use of an explicit DONE state (not even in the > examples you cite below). Instead the pattern I see is to not set an explicit > next_state_ when a state handler wants to exit the state machine. > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:129: for specific operations (e.g. |ReadHeaders|), and > b) an IO completion > |o_O| -> `o_O` > > https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... > net/docs/code-patterns.md:159: process, and calling `DoLoop()`. > Also, inspecting the return value of DoLoop and setting up the completion > callback. DoLoop() isn't aware of completion callbacks. lgtm to me from a high level. I would definitely reach for this doc if I had to touch any of these classes.
Helen: Sorry, I'm not sure how I missed your comments until Asanka pointed them out. Thank you for the drive-by. All (but Asanka specifically): I've incorporated Asanka's comments, which included a bit of high level reordering, and tried to be inspired by David's comments about adding examples in what I hope is a useful way. This is enough changed that I'd like someone to give it another thorough review--I'll arbitrarily pick Asanka since he was the last person to comment. But I welcome anyone else's comments. I think this is getting near landable :-}. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+blame/master/net/base/net_error_list.h#1)). On 2015/08/31 19:29:32, xunjieli wrote: > 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... Ah, nice, I didn't know about that feature of markdown. Thank you. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:39: The pattern usually used to construct state machines in the Chrome On 2015/08/31 19:29:32, xunjieli wrote: > usually [is] used? 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. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:79: multiple differnet events that complete outstanding IO, the framework On 2015/08/31 19:29:32, xunjieli wrote: > nit: s/differnet/different. I think this one disappeared in one of the editing rounds. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:81: receiving state much explicitly distinguish between those events and On 2015/08/31 19:29:32, xunjieli wrote: > s/much/must Ditto. https://codereview.chromium.org/1320933003/diff/1/net/docs/code-patterns.md#n... net/docs/code-patterns.md:84: 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). On 2015/08/28 16:24:57, mmenke wrote: > Maybe add HttpNetworkTransaction as well? I think those are the two > archetypical examples Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:8: [net_error_list.h](https://chromium.googlesource.com/chromium/src/+/master/net/base/net_error_list.h#1)). On 2015/09/04 14:51:03, asanka wrote: > Minor nit: I'd +1 Helen's suggestion to break this link. E.g.: > > ... indicates a network stack error code (see [net_error_list.h][]). > Zero indiates either net::OK or zero bytes read (usually EOF)... > > > (and somewhere else) > > [net_error_list.h]: https://chro.... Completely missed Helen's suggestion until you called it out--thank you. Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:11: an |int| return type. On 2015/09/04 14:51:03, asanka wrote: > Code is indicated using backticks. I.e. `int` instead of |int|. Here and > elsewhere. Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:25: bytes/EOF and possibly indicating net::OK, depending on context. On 2015/09/04 14:51:03, asanka wrote: > Nit: the problem with having to code format one identifier is that now you have > to code format other identifiers :P. I.e. `net::OK` instead of net::OK. Too true :-}. Done (I hope). https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:39: The DoLoop pattern is a pattern used in the network stack to construct On 2015/09/04 14:51:03, asanka wrote: > The DoLoop pattern is used in the network stack to ... Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:40: simple state machines. It is used for cases in which processing is On 2015/09/04 14:51:03, asanka wrote: > "simple" haha I'm meta amused. These state machines are *dirt* simple in the overall space of state machines, at least the ones I have experience with. Admittedly, I didn't realize it until I started to write this documentation, but the DoLoop state machines with all their hassle are the equivalent of a single function that blocks for IO completion. And wow can things get more complicated than that. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:67: states are named STATE`_<`STATENAME`>` (upper case separated by On 2015/09/04 14:51:03, asanka wrote: > Minor nit: STATE_<STATE_NAME> or STATE_<NAME_OF_STATE> would be clearer. http://www.guntheranderson.com/v/data/yourstat.htm :-}. Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:69: Those functions both take and return values that are either On 2015/09/04 14:51:03, asanka wrote: > This deviates from what I've seen. Notably, individual states often require the > previous result to be net::OK, and don't typically take a result as a parameter. > This is good thing since it makes the states not overly dependent on their > respective incoming edges, nor are they burdened with having to deal with an > error condition introduced by an unknown previous state. E.g. > > int DoLoop(int result) { > ... > case STATE_FOO: > DCHECK_EQ(result, OK); > result = DoFoo(); > break; > ... > } > > Split states, however, consist of a completion handler that *does* take a > result. This makes sense since the completion often depend on the result of the > async operation. It also eliminates the ambiguity of what the resulting int > means (byte count of what??). I.e.: > > ... > case STATE_FOO: > DCHECK_EQ(OK, result); > result = DoFoo(); > break; > > case STATE_FOO_COMPLETE: > result = DoFooComplete(result); > break; > ... Huh. Right you are, at least based on the two canonical examples in this doc. I've updated the doc; further comments welcome. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:108: `STATE_DONE` (by explicitly setting next_state_ to `STATE_DONE` On 2015/09/04 14:51:03, asanka wrote: > I don't see any consistent use of an explicit DONE state (not even in the > examples you cite below). Instead the pattern I see is to not set an explicit > next_state_ when a state handler wants to exit the state machine. Good point; oops. Writing documentation CLs is a learning experience :-}. I've modified the text to be a bit more flexible and suggest what I think is the common truth. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:129: for specific operations (e.g. |ReadHeaders|), and b) an IO completion On 2015/09/04 14:51:03, asanka wrote: > |o_O| -> `o_O` Done. https://codereview.chromium.org/1320933003/diff/120001/net/docs/code-patterns... net/docs/code-patterns.md:159: process, and calling `DoLoop()`. On 2015/09/04 14:51:03, asanka wrote: > Also, inspecting the return value of DoLoop and setting up the completion > callback. DoLoop() isn't aware of completion callbacks. Done.
LGTM https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:13: Many functions also have variables (often named `result`) containing Maybe `result` or `rv`? https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:14: such value; this is especially common in the [DoLoop](#DoLoop) pattern such value -> such a value https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:20: asynchronously. These functions generally return an int as described Should we mention void + taking a callback implies they return asynchronously? https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:25: bytes/EOF and possibly indicating `net::OK`, depending on context. and -> either...or ? https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:27: code specifying a synchronous failing return. "failing return" sounds a little weird. Maybe synchronous failing return -> synchronous failure? https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:30: provided will be retained by the called entity until completion, to "An IOBuffer provided" -> "A reference to any provided IOBuffer" https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:32: alive manually until asynchronous completion is signaled. 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. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:34: completion with the return value; if a callback is not provided, it " 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. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:41: basically single threaded and could be written in a single function, nit: single-threaded https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:42: if that function could block waiting for input. Generally initiation nit: I'd suggest a comma after Generally. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:43: of a state machine is triggerred by some method invocation by a class nit: triggered https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:47: is reset if completed and the consumer notified. "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? https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:106: * If a given state may complete synchronously or asynchronously (for example, This is also true if a given state always completes asynchronously. May not be worth mentioning. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:144: * The DoLoop mechanism is generally invoked in response to a consumer nit: Remove extra space https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:170: for specific operations (e.g. `ReadHeaders`), and b) an IO completion ReadHeaders() to match DoLoop()?
lgtm https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:8: [net_error_list.h] [net_error_list.h]). You can shorten this to [net_error_list.h][] where the link text will become the link ID. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:79: int DoFooComplete(); int DoFooComplete(int result); esp since it's invoked this way below. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:140: will have been set to the value proper for handling that incoming value proper -> proper value https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:214: CompletionCallback c = callback_; base::ResetAndReturn() appears to be the canonical way to handle callback_.
Thanks, Matt and Asanka! Holding off landing for one final go-round with Matt on the use of the "resetting the state machine" concept; happy to take any other comments other folks have in the meantime. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:8: [net_error_list.h] [net_error_list.h]). On 2015/09/21 20:31:01, asanka wrote: > You can shorten this to [net_error_list.h][] where the link text will become the > link ID. Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:13: Many functions also have variables (often named `result`) containing On 2015/09/21 19:46:28, mmenke wrote: > Maybe `result` or `rv`? Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:14: such value; this is especially common in the [DoLoop](#DoLoop) pattern On 2015/09/21 19:46:28, mmenke wrote: > such value -> such a value Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:20: asynchronously. These functions generally return an int as described On 2015/09/21 19:46:28, mmenke wrote: > Should we mention void + taking a callback implies they return asynchronously? 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. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:25: bytes/EOF and possibly indicating `net::OK`, depending on context. On 2015/09/21 19:46:28, mmenke wrote: > and -> either...or ? Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:27: code specifying a synchronous failing return. On 2015/09/21 19:46:28, mmenke wrote: > "failing return" sounds a little weird. Maybe synchronous failing return -> > synchronous failure? Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:30: provided will be retained by the called entity until completion, to On 2015/09/21 19:46:28, mmenke wrote: > "An IOBuffer provided" -> "A reference to any provided IOBuffer" Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:32: alive manually until asynchronous completion is signaled. On 2015/09/21 19:46:28, mmenke wrote: > 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. On reflection, agreed; this isn't worth mentioning because it isn't a common enough part of the pattern. Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:34: completion with the return value; if a callback is not provided, it On 2015/09/21 19:46:28, mmenke wrote: > " 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. Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:41: basically single threaded and could be written in a single function, On 2015/09/21 19:46:28, mmenke wrote: > nit: single-threaded Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:42: if that function could block waiting for input. Generally initiation On 2015/09/21 19:46:28, mmenke wrote: > nit: I'd suggest a comma after Generally. Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:43: of a state machine is triggerred by some method invocation by a class On 2015/09/21 19:46:28, mmenke wrote: > nit: triggered Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:47: is reset if completed and the consumer notified. On 2015/09/21 19:46:28, mmenke wrote: > "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? 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. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:79: int DoFooComplete(); On 2015/09/21 20:31:01, asanka wrote: > int DoFooComplete(int result); > > esp since it's invoked this way below. Ooops. Thank you. Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:106: * If a given state may complete synchronously or asynchronously (for example, On 2015/09/21 19:46:28, mmenke wrote: > This is also true if a given state always completes asynchronously. May not be > worth mentioning. I captured this by shortening the sentence to "If a given state may complete asynchronously". Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:140: will have been set to the value proper for handling that incoming On 2015/09/21 20:31:01, asanka wrote: > value proper -> proper value Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:144: * The DoLoop mechanism is generally invoked in response to a consumer On 2015/09/21 19:46:28, mmenke wrote: > nit: Remove extra space Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:170: for specific operations (e.g. `ReadHeaders`), and b) an IO completion On 2015/09/21 19:46:28, mmenke wrote: > ReadHeaders() to match DoLoop()? Done. https://codereview.chromium.org/1320933003/diff/140001/net/docs/code-patterns... net/docs/code-patterns.md:214: CompletionCallback c = callback_; On 2015/09/21 20:31:01, asanka wrote: > base::ResetAndReturn() appears to be the canonical way to handle callback_. Done.
https://codereview.chromium.org/1320933003/diff/160001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/160001/net/docs/code-patterns... net/docs/code-patterns.md:49: next consumer operation, or the legality of that operation. On 2015/09/22 01:38:21, rdsmith wrote: > On 2015/09/21 19:46:28, mmenke wrote: > > "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? > > 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. I think it's clearer just to say "at which point next_state_ is set to STATE_NONE and the consumer is notified", and remove the extra stuff you added. Also note that the "if completed" clause is redundant with the "until the operation requested by the method invocation completes", unless it adds meaning I'm not drawing from it.
Thanks for the feedback! Accepted, and landing. https://codereview.chromium.org/1320933003/diff/160001/net/docs/code-patterns.md File net/docs/code-patterns.md (right): https://codereview.chromium.org/1320933003/diff/160001/net/docs/code-patterns... net/docs/code-patterns.md:49: next consumer operation, or the legality of that operation. On 2015/09/22 14:44:49, mmenke wrote: > On 2015/09/22 01:38:21, rdsmith wrote: > > On 2015/09/21 19:46:28, mmenke wrote: > > > "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? > > > > 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. > > I think it's clearer just to say "at which point next_state_ is set to > STATE_NONE and the consumer is notified", and remove the extra stuff you added. > > Also note that the "if completed" clause is redundant with the "until the > operation requested by the method invocation completes", unless it adds meaning > I'm not drawing from it. Seems reasonable. I'll rely on the discussions of threading models in the bullet points below to carry the point I was raising. Done.
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, csharrison@chromium.org, asanka@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1320933003/#ps180001 (title: "Resolve "resetting state machine" wording.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320933003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320933003/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6c8f81939621e202bb3b4d8806382e9987bc7392 Cr-Commit-Position: refs/heads/master@{#350173} |
