QUIC - Whne alternate job decides to delay the main(TCP) job and if the

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 791a390dac5d8e5615822994b35f3e9cb3cd0cf6..d47af2b91309b1086514bf8b914b7770db326d5d 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -76,6 +76,15 @@ scoped_ptr<base::Value> NetLogHttpStreamJobCallback(
   return std::move(dict);
 }
 
+// Returns parameters associated with the delay of the HTTP stream job.
+scoped_ptr<base::Value> NetLogHttpStreamJobDelayCallback(
+    base::TimeDelta delay,
+    NetLogCaptureMode /* capture_mode */) {
+  scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
+  dict->SetInteger("resume_after_ms", static_cast<int>(delay.InMilliseconds()));
+  return std::move(dict);
+}
+
 // Returns parameters associated with the Proto (with NPN negotiation) of a HTTP
 // stream.
 scoped_ptr<base::Value> NetLogHttpStreamProtoCallback(
@@ -234,19 +243,35 @@ void HttpStreamFactoryImpl::Job::WaitFor(Job* job) {
   job->waiting_job_ = this;
 }
 
+void HttpStreamFactoryImpl::Job::ResumeWaitingJobAfterDelay(

[Ryan Hamilton, 2016/01/19 23:23:27]

Does this resume the current job, or a job which is waiting on this job? I
believe it's this job, right? In which case, I think I'd remove "WaitingJob"
from the method name.

[ramant (doing other things), 2016/01/20 04:37:52]

You are right.

Done.

+    const base::TimeDelta& delay) {

[Ryan Hamilton, 2016/01/19 23:23:27]

Instead if passing in delay here, an alternative would be to set wait_time_ and
use that exclusively. That would be nice, because it would explain why
wait_time_ is cleared after the task is posted. (Though perhaps that should be
cleared in the WaitComplete() method.

[ramant (doing other things), 2016/01/20 04:37:52]

Done.

+  DCHECK(!blocking_job_);
+  DCHECK_EQ(STATE_WAIT_FOR_JOB_COMPLETE, next_state_);
+
+  net_log_.AddEvent(NetLog::TYPE_HTTP_STREAM_JOB_DELAYED,
+                    base::Bind(&NetLogHttpStreamJobDelayCallback, delay));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, base::Bind(&HttpStreamFactoryImpl::Job::OnIOComplete,
+                            ptr_factory_.GetWeakPtr(), OK),
+      delay);
+  wait_time_ = base::TimeDelta();
+}
+
 void HttpStreamFactoryImpl::Job::Resume(Job* job,
                                         const base::TimeDelta& delay) {
   DCHECK_EQ(blocking_job_, job);
   blocking_job_ = NULL;
 
+  // If the job hasn't started, delay that job by |wait_time_| when it starts.

[Ryan Hamilton, 2016/01/19 23:23:27]

"that job" or "this job"?

[ramant (doing other things), 2016/01/20 04:37:52]

:-). It is |this| job. Updated the comment.

+  if (next_state_ == STATE_NONE && !delay.is_zero()) {
+    wait_time_ = delay;
+    return;
+  }
+
   // We know we're blocked if the next_state_ is STATE_WAIT_FOR_JOB_COMPLETE.
   // Unblock |this|.
-  if (next_state_ == STATE_WAIT_FOR_JOB_COMPLETE) {
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, base::Bind(&HttpStreamFactoryImpl::Job::OnIOComplete,
-                              ptr_factory_.GetWeakPtr(), OK),
-        delay);
-  }
+  if (next_state_ == STATE_WAIT_FOR_JOB_COMPLETE)
+    ResumeWaitingJobAfterDelay(delay);

[Ryan Hamilton, 2016/01/19 23:23:27]

Is the only case we need to schedule a future wait the case where we're at
STATE_NONE? What if we've made some progress but not all the way to waiting?

I think I might be inclined to write this as:

if (next_state_ == STATE_WAIT_FOR_JOB_COMPLETE)
    ResumeWaitingJobAfterDelay(delay)

if (next_state_ < STATE_WAIT_FOR_JOB) // or something
    wait_time_ = delay;

[ramant (doing other things), 2016/01/20 04:37:52]

+1 to the above and thanks for the very good point.

Done.

 }
 
 void HttpStreamFactoryImpl::Job::Orphan(const Request* request) {
@@ -827,7 +852,7 @@ int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
     return result;
   }
 
-  if (blocking_job_)
+  if (blocking_job_ || !wait_time_.is_zero())

[Ryan Hamilton, 2016/01/19 23:23:27]

Alternatively, we could always move to the WAIT_FOR_JOB state, and do this logic
there. I wonder if that might be cleaner. what do you think?

[ramant (doing other things), 2016/01/20 04:37:52]

Done.

     next_state_ = STATE_WAIT_FOR_JOB;
   else
     next_state_ = STATE_INIT_CONNECTION;
@@ -841,8 +866,15 @@ bool HttpStreamFactoryImpl::Job::ShouldForceQuic() const {
 }
 
 int HttpStreamFactoryImpl::Job::DoWaitForJob() {
-  DCHECK(blocking_job_);
   next_state_ = STATE_WAIT_FOR_JOB_COMPLETE;
+  if (wait_time_.is_zero()) {
+    DCHECK(blocking_job_);
+  } else {
+    // If there is a waiting_time, then resume the job after the wait_time_.
+    DCHECK(!blocking_job_);
+    ResumeWaitingJobAfterDelay(wait_time_);
+  }
+
   return ERR_IO_PENDING;
 }