[Cronet] Initial implementation of HttpURLConnection

components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.net.urlconnection;
+
+import java.io.IOException;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
+
+/**
+ * A package protected MessageLoop class for use in
+ * {@link CronetHttpURLConnection}.

[mmenke, 2014/12/03 16:21:01]

Should mention the mLoopTerminated behavior either here, or in loop()

[xunjieli, 2014/12/03 19:03:30]

Done.

+ */
+final class MessageLoop {

[mmenke, 2014/12/03 16:21:01]

Can we just make this implement Executor instead, and rename postTask to
execute?  (+ add @Override, of course).

Won't have to wrap it in an Executor, then, too

+    private final BlockingQueue<Runnable> mQueue;
+    // A reusable runnable to quit the message loop.
+    private final Runnable mQuitTask;
+    // Indicates whether this message loop is currently running.
+    private boolean mLoopRunning;
+    // Indicates whether the loop has been terminated. It cannot be started if
+    // it has been terminated.
+    private boolean mLoopTerminated;

[mmenke, 2014/12/03 16:21:01]

Suggest explicitly setting these to false - relying on objects being initialized
to null is one thing, but relying on primitive initialization strikes me as not
great style.

[mmenke, 2014/12/03 16:21:01]

Maybe "It cannot be started..." -> "It cannot be safely started..., because
there may be a pending quit task, which will result in exiting the next loop()
call without doing anything."

[mmenke, 2014/12/03 16:21:01]

I don't think "terminated" is a great term here, since every time we exit
loop(), the loop terminates.

Maybe mTaskFailed?

Also get rid of the terminated in comments.

[xunjieli, 2014/12/03 19:03:30]

Done. 

[xunjieli, 2014/12/03 19:03:30]

Done. I named it mWaitingInterrupted, as it is not really running the task
failed, but waiting for task has been interrupted.

[xunjieli, 2014/12/03 19:03:30]

Done.

+
+    public MessageLoop() {
+        mQueue = new LinkedBlockingQueue<Runnable>();
+        mQuitTask = new Runnable() {
+            @Override
+            public void run() {
+                mLoopRunning = false;
+            }
+        };
+    }
+
+    /**
+     * Runs the message loop. Be sure to call {@link MessageLoop#postQuitTask()}
+     * to end the loop.
+     * @throws IOException
+     */
+    public void loop() throws IOException {
+        if (mLoopTerminated) {
+            throw new IllegalStateException(
+                    "Cannot run loop as the loop has been terminated.");
+        }
+        if (mLoopRunning) {
+            throw new IllegalStateException(
+                    "Cannot run loop when it is already running.");
+        }
+        mLoopRunning = true;
+        while (mLoopRunning) {
+            try {
+                Runnable task = mQueue.take(); // Blocks if the queue is empty.
+                task.run();
+            } catch (InterruptedException e) {

[mmenke, 2014/12/03 16:21:01]

Can we test this?

[xunjieli, 2014/12/03 19:03:30]

Done.

+                mLoopRunning = false;
+                mLoopTerminated = true;
+                throw new IOException(e);
+            }

[mmenke, 2014/12/03 16:21:01]

Out of paranoia, maybe catch any RunTimeException, set mLoopTerminated to true,
and rethrow the exception unmodified as well?

[xunjieli, 2014/12/03 19:03:30]

But InterruptedException is not a runtime exception, it is a checked exception.
Wrapping InterruptedException in a IOEXception is because the get* methods in
HttpURLConnection only have throw IOException clauses. We need wrapping so we
can surface the exception to callers right? I changed this part a bit to catch
runtime exception, let me know if that looks better or I should revert.

+        }
+    }
+
+    /**
+     * Posts a reusable runnable, {@link #mQuitTask} to quit the loop. This
+     * causes the {@link #loop()} to terminate after processing all currently
+     * enqueued messages.
+     * @throws IOException
+     */
+    public void postQuitTask() throws IOException {
+        try {
+            mQueue.put(mQuitTask);
+        } catch (InterruptedException e) {
+            throw new IOException(e);
+        }
+    }
+
+    /**
+     * Posts a task to the message loop.
+     * @throws IOException
+     */
+    public void postTask(Runnable task) throws IOException {
+        if (task == null) {
+            throw new IllegalArgumentException();
+        }
+        try {
+            mQueue.put(task);
+        } catch (InterruptedException e) {
+            throw new IOException(e);
+        }
+    }
+}