Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
881133004:
QUIC - Race two connections. One connection which loads data from disk (Closed)
|
Created:
5 years, 10 months ago by ramant (doing other things) Modified:
5 years, 10 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQUIC - Race two connections. One connection which loads data from disk
cache sends a CHLO and another connection that doesn't wait to load
server config from disk cache and sends INCHOATE_HELLO.
This is not enabled. QuicStreamFactory tests with this flag enabled
and disabled. Tested chrome and all unit tests with this flag enabled.
R=rch@chromium.org
Committed: https://crrev.com/14abd31d803b6459e73a32092b3c46b17539db51
Cr-Commit-Position: refs/heads/master@{#315117}
Patch Set 1 : #Patch Set 2 : #
Total comments: 12
Patch Set 3 : Fix comments for Patch Set 2 #Patch Set 4 : main job doesn't load server config #Patch Set 5 : race two connection until job completion #Patch Set 6 : fix comments #Patch Set 7 : Cancel the job if disk cache doesn't have a server config #
Total comments: 20
Patch Set 8 : Fix comments #
Total comments: 12
Patch Set 9 : Fix comments for patch set 8 #Patch Set 10 : Merge with TOT, minor cleanup and unittest for racing #
Total comments: 11
Patch Set 11 : Fix comments for Patch Set 10 #
Total comments: 1
Messages
Total messages: 29 (14 generated)
rtenneti@chromium.org changed reviewers: + rch@chromium.org
Patchset #1 (id:1) has been deleted
Hi Ryan, Implemented racing of QUIC connections. Tested chromium by enabling and disabling the connection racing. Disabled the flag. Will submit another CL that enables this via finch experiment. QuciStreamFactoryTests run for combinations QUIC versions and enable_connection_racing_ flag. PTAL. thanks raman
https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:184: bool is_cancelled() const { return is_cancelled_; } nit: newline after https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:263: server_info_->CancelWaitForDataReadyCallback(); Out of curiosity, did we need this code before? https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:459: } This seems to be the only place where a job is cancelled. Does this mean a main job can not be cancelled? That's surprising to me. I would have expected that if we start two jobs, the only time we cancel a job is when on of the jobs finishes. Does that seem plausible or does that actually cause problems? https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:722: return OK; If this returns OK, it looks like we skip the code on like 730. Is that expected? https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:798: } else if (job->is_cancelled() && (!active_jobs_[server_id].empty())) { Can rv == OK if the job was cancelled? In this case is |job| still in active_jobs_? https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.h:218: int CreateAuxilaryJob(const QuicServerId server_id, This is not super important, but I thought you decided to make the main job to skip the read and have the aux job do the waiting?
Patchset #3 (id:60001) has been deleted
Hi Ryan, Uploaded multiple implementations of racing of server config jobs. + One approach (uses less resources) - stop as soon as a server config is loaded. + Race two connections (don't race only if loading of server config from disk cache looses). + If disk cache loads server config without IO_PENDING (or if we get server config from memory), then we send that server config (instead of sending INCHOATE_HELLO). Would like to do an experiment where we dont load server config from disk cache if that does better on all platforms. PTAL. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:184: bool is_cancelled() const { return is_cancelled_; } On 2015/02/03 18:54:36, Ryan Hamilton wrote: > nit: newline after Done. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:263: server_info_->CancelWaitForDataReadyCallback(); On 2015/02/03 18:54:36, Ryan Hamilton wrote: > Out of curiosity, did we need this code before? If we had closed the job when it is in the WaitForDataReady (I think we never did that before). Deleted this code. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:459: } On 2015/02/03 18:54:36, Ryan Hamilton wrote: > This seems to be the only place where a job is cancelled. Does this mean a main > job can not be cancelled? That's surprising to me. Cancelled the job that loads server config from disk cache (if it looses). > > I would have expected that if we start two jobs, the only time we cancel a job > is when on of the jobs finishes. Does that seem plausible or does that actually > cause problems? Originally cancelled the job as soon as we have a server config. This avoids two certificate verifications/proof/channel_id checks happening on the client (to reduce the work we do on Android/client. Was feeling guilty to use resources on Android). Changed the code to race until job completion. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:722: return OK; On 2015/02/03 18:54:36, Ryan Hamilton wrote: > If this returns OK, it looks like we skip the code on like 730. Is that > expected? Removed the duplicated code from lines 750-753. Done. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.cc:798: } else if (job->is_cancelled() && (!active_jobs_[server_id].empty())) { On 2015/02/03 18:54:36, Ryan Hamilton wrote: > Can rv == OK if the job was cancelled? In this case is |job| still in > active_jobs_? Previously STLDeleteElements was deleting all the jobs. Changed the code to delete cancelled job right away. https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/881133004/diff/40001/net/quic/quic_stream_fac... net/quic/quic_stream_factory.h:218: int CreateAuxilaryJob(const QuicServerId server_id, On 2015/02/03 18:54:36, Ryan Hamilton wrote: > This is not super important, but I thought you decided to make the main job to > skip the read and have the aux job do the waiting? Done.
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:260: GetParam().enable_connection_racing); I'm a bit surprised that these tests pass with connection racing. I guess that happens because we never actually end up starting a second connection? https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:326: // callback. nit: I think I'd reverse the order here: // If we are waiting for WaitForDataReadyCallback, then cancel the pending // callback. if (server_info_ && io_state_ 0= STATE_LOAD_SERVER_INFO_COMPLETE) server_info_->CancelWaitForDataReadyCallback(); } This makes it one line shorter and makes it clear what the "if we are waiting for..." part of the comment applies to (namely the io_state_). https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:452: } If this is the right place to check this value, I'd do it outside of the "if (enable_racing())" check. That being said, I'm concerned that this is not the only place we need to check this value. Should we check this in DoLoop? Perhaps there should be a new state STATE_CANCELLED which Cancel() sets io_state_ to? https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:720: } Do you think it would make sense to delay starting the "no waiting" job until the "wait for disk cache job" has resolved the host? https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:788: DCHECK_LT(0u, jobs->size()); // Verify there is atleast one job. It looks like jobs might be null. in that case, this DCHECK would be ... bad :> What's the invariant about when cancelled jobs must be removed from active_jobs? Is it as soon as they're cancelled? https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:830: STLDeleteElements(&(active_jobs_[server_id])); // Delete all jobs. nit: I think this two comments are pretty redundant with the names of the methods. I'd probably nuke 'em https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:997: for (JobSet::iterator it = active_jobs_[server_id].begin(); nit: for (Job* job : active_jobs_[server_id]) { ... } https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:1000: other_job = *it; I think I'd be inclined to do this: CancelOtherJobs(...) { for (Job* other_job : active_jobs_[server_id]) { if (other_job == job || other_job->is_cancelled()) continue; other_job->Cancel(); } I think I would move the SendConnectionClose() logic into the Cancel() method. Also, consider a new error code for this condition. Not required but might be a good idea. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:1008: other_job->CancelJob(); It looks like CancelJob does not invoke the callback to notify the factory that the job is complete. Will something notify the factory about this job?
Patchset #9 (id:220001) has been deleted
Patchset #8 (id:200001) has been deleted
Did what we had talked offline. PTAL. Thanks very much raman https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:260: GetParam().enable_connection_racing); On 2015/02/04 20:10:29, Ryan Hamilton wrote: > I'm a bit surprised that these tests pass with connection racing. I guess that > happens because we never actually end up starting a second connection? Acknowledged. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:326: // callback. On 2015/02/04 20:10:29, Ryan Hamilton wrote: > nit: I think I'd reverse the order here: > > // If we are waiting for WaitForDataReadyCallback, then cancel the pending > // callback. > if (server_info_ && io_state_ 0= STATE_LOAD_SERVER_INFO_COMPLETE) > server_info_->CancelWaitForDataReadyCallback(); > } > > This makes it one line shorter and makes it clear what the "if we are waiting > for..." part of the comment applies to (namely the io_state_). Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:452: } On 2015/02/04 20:10:29, Ryan Hamilton wrote: > If this is the right place to check this value, I'd do it outside of the "if > (enable_racing())" check. That being said, I'm concerned that this is not the > only place we need to check this value. Should we check this in DoLoop? Perhaps > there should be a new state STATE_CANCELLED which Cancel() sets io_state_ to? The only time, we cancel the job is when we wait (thus start another job) and the other job got the server config from disk. This job will be deleted when the other job finishes (we delete the jobs when OnJobComplete finishes. Did this, as we had talked offline to reduce the number of places where we delete the object.). Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:720: } On 2015/02/04 20:10:29, Ryan Hamilton wrote: > Do you think it would make sense to delay starting the "no waiting" job until > the "wait for disk cache job" has resolved the host? Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:788: DCHECK_LT(0u, jobs->size()); // Verify there is atleast one job. On 2015/02/04 20:10:29, Ryan Hamilton wrote: > It looks like jobs might be null. in that case, this DCHECK would be ... bad :> > What's the invariant about when cancelled jobs must be removed from active_jobs? > Is it as soon as they're cancelled? In the previous implementation, removed the jobs as soon as they are cancelled and OnJobComplete was called for those jobs. If there is more than one job, we cancel that job (and all the callbacks) and delete the job. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:830: STLDeleteElements(&(active_jobs_[server_id])); // Delete all jobs. On 2015/02/04 20:10:29, Ryan Hamilton wrote: > nit: I think this two comments are pretty redundant with the names of the > methods. I'd probably nuke 'em Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:997: for (JobSet::iterator it = active_jobs_[server_id].begin(); On 2015/02/04 20:10:29, Ryan Hamilton wrote: > nit: for (Job* job : active_jobs_[server_id]) { > ... > } Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:1000: other_job = *it; On 2015/02/04 20:10:29, Ryan Hamilton wrote: > I think I'd be inclined to do this: > > CancelOtherJobs(...) { > for (Job* other_job : active_jobs_[server_id]) { > if (other_job == job || other_job->is_cancelled()) > continue; > other_job->Cancel(); > } > > I think I would move the SendConnectionClose() logic into the Cancel() method. > Also, consider a new error code for this condition. Not required but might be a > good idea. Done. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:1008: other_job->CancelJob(); On 2015/02/04 20:10:29, Ryan Hamilton wrote: > It looks like CancelJob does not invoke the callback to notify the factory that > the job is complete. Will something notify the factory about this job? Deleted this method. Factory is cancelling the job and deleting the job. In the other case, when we call CancelJob, we call Factory's OnJobComplete.
Patchset #9 (id:260001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Hi Ryan, Tested loading all properties multiple times with connection racing enabled and disabled. Cleaned up code from last night. PTAL. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:260: GetParam().enable_connection_racing); On 2015/02/05 03:39:41, ramant wrote: > On 2015/02/04 20:10:29, Ryan Hamilton wrote: > > I'm a bit surprised that these tests pass with connection racing. I guess that > > happens because we never actually end up starting a second connection? > > Acknowledged. Undid the changes to this file. https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/180001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:788: DCHECK_LT(0u, jobs->size()); // Verify there is atleast one job. On 2015/02/05 03:39:41, ramant wrote: > On 2015/02/04 20:10:29, Ryan Hamilton wrote: > > It looks like jobs might be null. in that case, this DCHECK would be ... bad > :> > > What's the invariant about when cancelled jobs must be removed from > active_jobs? > > Is it as soon as they're cancelled? > > In the previous implementation, removed the jobs as soon as they are cancelled > and OnJobComplete was called for those jobs. > > If there is more than one job, we cancel that job (and all the callbacks) and > delete the job. Checked that there is at least one other job to complete the request. The job that finishes the request, deletes all the jobs.
I need to run off to a tech-talk, but here are the thoughts I have for now. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_protocol.... net/quic/quic_protocol.h:517: QUIC_LOAD_SERVER_CONFIG_JOB_CANCELLED = 70, nit: I would just name this QUIC_CONNECTION_CANCELLED https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:208: base::StringPiece method_; instead of doing this, let's change the Job() constructor to just take bool is_post https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:334: started_another_job_ = true; Does started_nother_job_ mean that this job started another job, or that this job was started by another job, or both? It looks like the only time it's set to true is here, where |this| is the aux job. But down on 476 is looks like we expect it to be true in the main job. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:478: !factory_->CryptoConfigCacheIsEmpty(server_id_))) { Instead of tracking this here, would it make more sense to do this is DoLoadServerInfoComplete()? I think that in DoResolveHostComplete, we could check if we have server_info_ and if not, skip straight to STATE_CONNECTION. This would skip DoLoadServerInfo and DoLoadServerInfoComplete when we have no server info. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:482: CancelJob(); Instead of invoking CancelJob() can we simply return with the ERROR below and rely on OnJobComplete() to do the right cleanup? https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:796: return; Do we need to move this job from active_jobs_?
Made all the changes we had discussed offline (to Cancel the job). PTAL. Thanks very much raman https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_protocol.... net/quic/quic_protocol.h:517: QUIC_LOAD_SERVER_CONFIG_JOB_CANCELLED = 70, On 2015/02/05 20:30:27, Ryan Hamilton wrote: > nit: I would just name this QUIC_CONNECTION_CANCELLED Done. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:208: base::StringPiece method_; On 2015/02/05 20:30:28, Ryan Hamilton wrote: > instead of doing this, let's change the Job() constructor to just take bool > is_post Done. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:334: started_another_job_ = true; On 2015/02/05 20:30:28, Ryan Hamilton wrote: > Does started_nother_job_ mean that this job started another job, or that this > job was started by another job, or both? It looks like the only time it's set to > true is here, where |this| is the aux job. But down on 476 is looks like we > expect it to be true in the main job. Done. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:478: !factory_->CryptoConfigCacheIsEmpty(server_id_))) { On 2015/02/05 20:30:28, Ryan Hamilton wrote: > Instead of tracking this here, would it make more sense to do this is > DoLoadServerInfoComplete()? > > I think that in DoResolveHostComplete, we could check if we have server_info_ > and if not, skip straight to STATE_CONNECTION. This would skip DoLoadServerInfo > and DoLoadServerInfoComplete when we have no server info. Done. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:482: CancelJob(); On 2015/02/05 20:30:28, Ryan Hamilton wrote: > Instead of invoking CancelJob() can we simply return with the ERROR below and > rely on OnJobComplete() to do the right cleanup? Done. https://codereview.chromium.org/881133004/diff/320001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:796: return; On 2015/02/05 20:30:28, Ryan Hamilton wrote: > Do we need to move this job from active_jobs_? Done.
Patchset #10 (id:360001) has been deleted
Patchset #10 (id:380001) has been deleted
Patchset #10 (id:290008) has been deleted
Looking great! https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:339: CancelWaitForDataReadyPendingCallback(); Does this call itself? https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:424: factory_->CreateAuxilaryJob(server_id_, is_post_, net_log_); Nice! This block of code is clear, and obvious. Love it. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:776: it != job_requests_map_[server_id].end(); ++it) { nit: As long as you're here I think you can probably clean this up with a range-based for loop: for (Request request : job_requests_map_[server_id]) { ... } https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:781: JobSet* jobs = &(active_jobs_[server_id]); Can you add a comment here explaining what this code is doing? Totally your call, but I tend to prefer errors to be handled at the top of a method so I'd probably write this as: if (rv != OK) { ... } if (rv == OK) { ... } But either approach is fine with me. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:979: Did this move to match the order in the .h file?
Thanks very much for your comments Ryan. PTAL. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:172: void CancelWaitForDataReadyPendingCallback(); Deleted this method. The name of the function was too confusing. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:339: CancelWaitForDataReadyPendingCallback(); On 2015/02/06 18:22:47, Ryan Hamilton wrote: > Does this call itself? Undid this change. Restored the original code. IMO, names were too confusing. Same method name on Job as well as server_info (and one has the word "Pending" in it etc). We could call disk cache's CancelWaitForDataReadyCallback twice (https://code.google.com/p/chromium/codesearch#chromium/src/net/http/disk_cach...) https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:424: factory_->CreateAuxilaryJob(server_id_, is_post_, net_log_); On 2015/02/06 18:22:47, Ryan Hamilton wrote: > Nice! This block of code is clear, and obvious. Love it. Thanks much. Both of us are making this code better. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:776: it != job_requests_map_[server_id].end(); ++it) { On 2015/02/06 18:22:48, Ryan Hamilton wrote: > nit: As long as you're here I think you can probably clean this up with a > range-based for loop: > > for (Request request : job_requests_map_[server_id]) { > ... > } +1 to the above. Thanks much. Done. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:781: JobSet* jobs = &(active_jobs_[server_id]); On 2015/02/06 18:22:47, Ryan Hamilton wrote: > Can you add a comment here explaining what this code is doing? > > Totally your call, but I tend to prefer errors to be handled at the top of a > method so I'd probably write this as: > > if (rv != OK) { > ... > } > > if (rv == OK) { > ... > } > > But either approach is fine with me. Done. https://codereview.chromium.org/881133004/diff/410001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:979: On 2015/02/06 18:22:47, Ryan Hamilton wrote: > Did this move to match the order in the .h file? Acknowledged. https://codereview.chromium.org/881133004/diff/430001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/881133004/diff/430001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:321: if (rv != ERR_IO_PENDING) If auxilary job finishes without IO_PENDING, called OnJobComplete (in case it is an error. Making it similar to lines 313-314).
lgtm thanks for getting this done!
On 2015/02/06 19:48:32, Ryan Hamilton wrote: > lgtm > > thanks for getting this done! Many many thanks Ryan for your review/peer programming.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881133004/430001
Message was sent while issue was closed.
Committed patchset #11 (id:430001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/14abd31d803b6459e73a32092b3c46b17539db51 Cr-Commit-Position: refs/heads/master@{#315117} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
