Initial implementation of CronetBidirectionalStream.

components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java

 // Copyright 2015 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;
 
+import android.os.ConditionVariable;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import org.chromium.base.test.util.Feature;
+import org.chromium.net.CronetTestBase.OnlyRunNativeCronet;
+import org.chromium.net.TestBidirectionalStreamCallback.FailureType;
+import org.chromium.net.TestBidirectionalStreamCallback.ResponseStep;
 
 import java.nio.ByteBuffer;
-import java.util.concurrent.Executor;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ThreadFactory;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 /**
  * Test functionality of BidirectionalStream interface.
  */
 public class BidirectionalStreamTest extends CronetTestBase {
     private CronetTestFramework mTestFramework;
 
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-        mTestFramework = startCronetTestFramework();
-        assertTrue(NativeTestServer.startNativeTestServer(getContext()));
-        // Add url interceptors after native application context is initialized.
-        MockUrlRequestJobFactory.setUp();
+        // Load library first to create MockCertVerifier.
+        System.loadLibrary("cronet_tests");
+        CronetEngine.Builder builder = new CronetEngine.Builder(getContext());
+        builder.setMockCertVerifierForTesting(QuicTestServer.createMockCertVerifier());
+
+        mTestFramework = startCronetTestFrameworkWithUrlAndCronetEngineBuilder(null, builder);
+        assertTrue(Http2TestServer.startHttp2TestServer(
+                getContext(), QuicTestServer.getServerCert(), QuicTestServer.getServerCertKey()));
     }
 
     @Override
     protected void tearDown() throws Exception {
-        NativeTestServer.shutdownNativeTestServer();
-        mTestFramework.mCronetEngine.shutdown();
+        assertTrue(Http2TestServer.shutdownHttp2TestServer());
+        if (mTestFramework.mCronetEngine != null) {
+            mTestFramework.mCronetEngine.shutdown();
+        }
         super.tearDown();
     }
 
-    private class TestBidirectionalStreamCallback extends BidirectionalStream.Callback {
-        // Executor Service for Cronet callbacks.
-        private final ExecutorService mExecutorService =
-                Executors.newSingleThreadExecutor(new ExecutorThreadFactory());
-        private Thread mExecutorThread;
+    private TestBidirectionalStreamCallback startAndWaitForComplete(String url) throws Exception {

[xunjieli, 2016/01/27 16:16:38]

Only used in two tests. Don't think we will use in more tests? Suggest inline.
Easier to see that way.

[mef, 2016/01/27 19:06:47]

Done.

+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
 
-        private class ExecutorThreadFactory implements ThreadFactory {
-            public Thread newThread(Runnable r) {
-                mExecutorThread = new Thread(r);
-                return mExecutorThread;
-            }
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .setHttpMethod("GET")
+                        .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        return callback;
+    }
+
+    private static void checkResponseInfo(UrlResponseInfo responseInfo, String expectedUrl,
+            int expectedHttpStatusCode, String expectedHttpStatusText) {
+        assertEquals(expectedUrl, responseInfo.getUrl());
+        assertEquals(
+                expectedUrl, responseInfo.getUrlChain().get(responseInfo.getUrlChain().size() - 1));
+        assertEquals(expectedHttpStatusCode, responseInfo.getHttpStatusCode());
+        assertEquals(expectedHttpStatusText, responseInfo.getHttpStatusText());
+        assertFalse(responseInfo.wasCached());
+        assertTrue(responseInfo.toString().length() > 0);
+    }
+
+    private static String makeLongString(String base, int repetition) {

[xunjieli, 2016/01/27 16:16:38]

nit: consider s/make/create to be consistent with other method names.

[mef, 2016/01/27 19:06:47]

Done.

+        StringBuilder builder = new StringBuilder(base.length() * repetition);
+        for (int i = 0; i < repetition; ++i) {
+            builder.append(i);
+            builder.append(base);
         }
+        return builder.toString();
+    }
 
-        @Override
-        public void onRequestHeadersSent(BidirectionalStream stream) {}
-
-        @Override
-        public void onResponseHeadersReceived(BidirectionalStream stream, UrlResponseInfo info) {}
-
-        @Override
-        public void onReadCompleted(
-                BidirectionalStream stream, UrlResponseInfo info, ByteBuffer buffer) {}
-
-        @Override
-        public void onWriteCompleted(
-                BidirectionalStream stream, UrlResponseInfo info, ByteBuffer buffer) {}
-
-        @Override
-        public void onResponseTrailersReceived(BidirectionalStream stream, UrlResponseInfo info,
-                UrlResponseInfo.HeaderBlock trailers) {}
-
-        @Override
-        public void onSucceeded(BidirectionalStream stream, UrlResponseInfo info) {}
-
-        @Override
-        public void onFailed(
-                BidirectionalStream stream, UrlResponseInfo info, CronetException error) {}
-
-        @Override
-        public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) {}
-
-        Executor getExecutor() {
-            return mExecutorService;
+    private static UrlResponseInfo createUrlResponseInfo(
+            String[] urls, String message, int statusCode, int receivedBytes, String... headers) {
+        ArrayList<Map.Entry<String, String>> headersList = new ArrayList<>();
+        for (int i = 0; i < headers.length; i += 2) {
+            headersList.add(new AbstractMap.SimpleImmutableEntry<String, String>(
+                    headers[i], headers[i + 1]));
         }
+        UrlResponseInfo unknown = new UrlResponseInfo(
+                Arrays.asList(urls), statusCode, message, headersList, false, "h2", null);
+        unknown.setReceivedBytesCount(receivedBytes);
+        return unknown;

[xunjieli, 2016/01/27 16:16:38]

nit: I am not sure why the variable is called "unknown." Maybe simply name it as
urlRequestInfo. The method is straightforward, but the variable name made me
think twice.

[mef, 2016/01/27 19:06:47]

Done.

     }
 
     @SmallTest
     @Feature({"Cronet"})
     public void testBuilderChecks() throws Exception {
         TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
         try {
             new BidirectionalStream.Builder(
                     null, callback, callback.getExecutor(), mTestFramework.mCronetEngine);
             fail("URL not null-checked");
         } catch (NullPointerException e) {
             assertEquals("URL is required.", e.getMessage());
         }
         try {
-            new BidirectionalStream.Builder(NativeTestServer.getRedirectURL(), null,
+            new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), null,
                     callback.getExecutor(), mTestFramework.mCronetEngine);
             fail("Callback not null-checked");
         } catch (NullPointerException e) {
             assertEquals("Callback is required.", e.getMessage());
         }
         try {
-            new BidirectionalStream.Builder(NativeTestServer.getRedirectURL(), callback, null,
-                    mTestFramework.mCronetEngine);
+            new BidirectionalStream.Builder(
+                    Http2TestServer.getServerUrl(), callback, null, mTestFramework.mCronetEngine);
             fail("Executor not null-checked");
         } catch (NullPointerException e) {
             assertEquals("Executor is required.", e.getMessage());
         }
         try {
             new BidirectionalStream.Builder(
-                    NativeTestServer.getRedirectURL(), callback, callback.getExecutor(), null);
+                    Http2TestServer.getServerUrl(), callback, callback.getExecutor(), null);
             fail("CronetEngine not null-checked");
         } catch (NullPointerException e) {
             assertEquals("CronetEngine is required.", e.getMessage());
         }
         // Verify successful creation doesn't throw.
-        new BidirectionalStream.Builder(NativeTestServer.getRedirectURL(), callback,
-                callback.getExecutor(), mTestFramework.mCronetEngine);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        try {
+            builder.addHeader(null, "value");
+            fail("Header name is not null-checked");
+        } catch (NullPointerException e) {
+            assertEquals("Invalid header name.", e.getMessage());
+        }
+        try {
+            builder.addHeader("name", null);
+            fail("Header value is not null-checked");
+        } catch (NullPointerException e) {
+            assertEquals("Invalid header value.", e.getMessage());
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testFailPlainHttp() throws Exception {
+        String url = "http://example.com";
+        TestBidirectionalStreamCallback callback = startAndWaitForComplete(url);
+        assertEquals("Exception in BidirectionalStream: net::ERR_DISALLOWED_URL_SCHEME",
+                callback.mError.getMessage());
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testSimpleGet() throws Exception {
+        String url = Http2TestServer.getEchoMethodUrl();
+        TestBidirectionalStreamCallback callback = startAndWaitForComplete(url);
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        // Default method is 'GET'.
+        assertEquals("GET", callback.mResponseAsString);
+        UrlResponseInfo urlResponseInfo =
+                createUrlResponseInfo(new String[] {url}, "", 200, 27, ":status", "200");
+        assertResponseEquals(urlResponseInfo, callback.mResponseInfo);
+        checkResponseInfo(callback.mResponseInfo, Http2TestServer.getEchoMethodUrl(), 200, "");
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testSimpleHead() throws Exception {
+        String url = Http2TestServer.getEchoMethodUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .setHttpMethod("HEAD")
+                        .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("HEAD", callback.mResponseAsString);
+        UrlResponseInfo urlResponseInfo =
+                createUrlResponseInfo(new String[] {url}, "", 200, 28, ":status", "200");
+        assertResponseEquals(urlResponseInfo, callback.mResponseInfo);
+        checkResponseInfo(callback.mResponseInfo, Http2TestServer.getEchoMethodUrl(), 200, "");
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testSimplePost() throws Exception {
+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.addWriteData("Test String".getBytes());
+        callback.addWriteData("1234567890".getBytes());
+        callback.addWriteData("woot!".getBytes());
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .addHeader("foo", "bar")
+                        .addHeader("empty", "")
+                        .addHeader("Content-Type", "zebra")
+                        .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("Test String1234567890woot!", callback.mResponseAsString);
+        assertEquals("bar", callback.mResponseInfo.getAllHeaders().get("echo-foo").get(0));
+        assertEquals("", callback.mResponseInfo.getAllHeaders().get("echo-empty").get(0));
+        assertEquals(
+                "zebra", callback.mResponseInfo.getAllHeaders().get("echo-content-type").get(0));
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testSetHttpMethod() throws Exception {

[xunjieli, 2016/01/27 16:16:38]

Suggest naming this test as testSimplePut. And move the setHttpMethod(null) part
to testBuilderChecks. 

[mef, 2016/01/27 19:06:47]

Done.

+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.addWriteData("Put This Data!".getBytes());
+        String methodName = "PUT";
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        // Try to set 'null' method.
+        try {
+            builder.setHttpMethod(null);
+            fail("Exception not thrown");
+        } catch (NullPointerException e) {
+            assertEquals("Method is required.", e.getMessage());
+        }
+        builder.setHttpMethod(methodName);
+        builder.build().start();
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("Put This Data!", callback.mResponseAsString);
+        assertEquals(methodName, callback.mResponseInfo.getAllHeaders().get("echo-method").get(0));
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testBadMethod() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        try {
+            builder.setHttpMethod("bad:method!");
+            builder.build().start();
+            fail("IllegalArgumentException not thrown.");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Invalid http method bad:method!", e.getMessage());
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testBadHeaderName() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        try {
+            builder.addHeader("goodheader1", "headervalue");
+            builder.addHeader("header:name", "headervalue");
+            builder.addHeader("goodheader2", "headervalue");
+            builder.build().start();
+            fail("IllegalArgumentException not thrown.");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Invalid header header:name=headervalue", e.getMessage());
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testBadHeaderValue() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getServerUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        try {
+            builder.addHeader("headername", "bad header\r\nvalue");
+            builder.build().start();
+            fail("IllegalArgumentException not thrown.");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Invalid header headername=bad header\r\nvalue", e.getMessage());
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testAddHeader() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        String headerName = "header-name";
+        String headerValue = "header-value";
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoHeaderUrl(headerName),
+                        callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+        builder.addHeader(headerName, headerValue);
+        builder.setHttpMethod("GET");
+        builder.build().start();
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals(headerValue, callback.mResponseAsString);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testMultiRequestHeaders() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        String headerName = "header-name";
+        String headerValue1 = "header-value1";
+        String headerValue2 = "header-value2";
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoAllHeadersUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        builder.addHeader(headerName, headerValue1);
+        builder.addHeader(headerName, headerValue2);
+        builder.setHttpMethod("GET");
+        builder.build().start();
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        String headers = callback.mResponseAsString;
+        Pattern pattern = Pattern.compile(headerName + ":\\s(.*)\\r\\n");
+        Matcher matcher = pattern.matcher(headers);
+        List<String> actualValues = new ArrayList<String>();
+        while (matcher.find()) {
+            actualValues.add(matcher.group(1));
+        }
+        assertEquals(1, actualValues.size());
+        assertEquals("header-value2", actualValues.get(0));
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testEchoTrailers() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        String headerName = "header-name";
+        String headerValue = "header-value";
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoTrailersUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        builder.addHeader(headerName, headerValue);
+        builder.setHttpMethod("GET");
+        builder.build().start();
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertNotNull(callback.mTrailers);
+        assertEquals(headerValue, callback.mTrailers.getAsMap().get("echo-header-name").get(0));

[xunjieli, 2016/01/27 16:16:38]

I am not sure what "echo-header-name" came from, I was expectating
|headerValue|. Might use a comment.

[mef, 2016/01/27 19:06:47]

Done.

+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testCustomUserAgent() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        String userAgentName = "User-Agent";
+        String userAgentValue = "User-Agent-Value";
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoHeaderUrl(userAgentName),
+                        callback, callback.getExecutor(), mTestFramework.mCronetEngine);
+        builder.setHttpMethod("GET");
+        builder.addHeader(userAgentName, userAgentValue);
+        builder.build().start();
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals(userAgentValue, callback.mResponseAsString);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testEchoStream() throws Exception {

[xunjieli, 2016/01/27 16:16:38]

This test seems to be a combination of some other tests. Is it redundant and
same as  testSimplePost?

[mef, 2016/01/27 19:06:47]

This was THE original test of bidirectional streaming. Among other things it is
useful for verifying that data split between multiple frames is sent / received
properly.

+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        String[] testData = {"Test String", makeLongString("1234567890", 50000), "woot!"};
+        StringBuilder stringData = new StringBuilder();
+        for (String writeData : testData) {
+            callback.addWriteData(writeData.getBytes());
+            stringData.append(writeData);
+        }
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .addHeader("foo", "Value with Spaces")
+                        .addHeader("Content-Type", "zebra")
+                        .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals(stringData.toString(), callback.mResponseAsString);
+        assertEquals(
+                "Value with Spaces", callback.mResponseInfo.getAllHeaders().get("echo-foo").get(0));
+        assertEquals(
+                "zebra", callback.mResponseInfo.getAllHeaders().get("echo-content-type").get(0));
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testEchoStreamEmptyWrite() throws Exception {
+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.addWriteData(new byte[0]);
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .build();
+        stream.start();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("", callback.mResponseAsString);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testDoubleWrite() throws Exception {
+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        callback.addWriteData("1".getBytes());
+        callback.addWriteData("2".getBytes());
+        callback.addWriteData("3".getBytes());
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getBlockingDirectExecutor(),

[xunjieli, 2016/01/26 21:41:10]

I am not sure that executing the callbacks on the network thread fully exercises
our API contract, since processing the OnWrite callback means pausing the
network thread (What we really want is to pause the executor thread, but keep
calling Write from the Java test thread which will then successfully post the
write task to the network thread). Can we use the executor that you made, but
instead running the task on current thread (which is the network thread), and
make the task run another thread? (like creating a new thread and stored it in
mExecutorThread, and execute the runnable on mExecutorThread?)

[mef, 2016/01/27 19:06:47]

Done. Initially I've used direct executor to prevent callback posted on another
thread from switching state to 'waiting_for_write'.
Your comment made me realize that calling read or write second time from the
callback invoked on executor thread will be guaranteed to throw because even if
first operation completes on network thread, the onComplete callback is posted
to the executor and will not get invoked until completion of previous callback
task.

+                                               mTestFramework.mCronetEngine)
+                        .build();
+        stream.start();
+        callback.waitForNextWriteStep();
+        // Executor will block the network thread posting the callback.
+        callback.closeDirectExecutorBlock();
+        // First write is allowed as there are no other writes in flight.
+        callback.startNextWrite(stream);
+        try {
+            // Second write may be allowed because onWriteCompleted callback is posted after state

[xunjieli, 2016/01/26 21:41:10]

This doesn't seem very intuitive. We should be able to report the error on the
second write.. 

[mef, 2016/01/27 19:06:47]

Done.

+            // change.
+            callback.startNextWrite(stream);
+            // Third write must throw an exception, because executor is blocked in 'WRITING' state.
+            callback.startNextWrite(stream);
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Unexpected write attempt.", e.getMessage());
+        }
+
+        callback.setAutoAdvance(true);
+        // Open the executor. Stream should continue.
+        callback.openDirectExecutorBlock();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testDoubleRead() throws Exception {
+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        String stringData = "12345";
+        callback.addWriteData(stringData.getBytes());
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getBlockingDirectExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .build();
+        stream.start();
+        callback.waitForNextReadStep();
+        callback.startNextWrite(stream);
+        callback.waitForNextWriteStep();
+        // Executor will block the network thread posting the callback.
+        callback.closeDirectExecutorBlock();
+        // Read next chunk of test data.
+        ByteBuffer readBuffer = ByteBuffer.allocateDirect(1);
+        // First read is allowed as there are no other read in flight.
+        callback.startNextRead(stream, readBuffer);
+        try {
+            // Second read may be allowed because onReadCompleted callback is posted after state
+            // change.
+            callback.startNextRead(stream, readBuffer);
+            // Third read must throw an exception, becuase executor is blocked in 'READING' state.
+            callback.startNextRead(stream, readBuffer);
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Unexpected read attempt.", e.getMessage());
+        }
+
+        callback.setAutoAdvance(true);
+        // Open the executor. Stream should continue.
+        callback.openDirectExecutorBlock();
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(stringData, callback.mResponseAsString);
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testEchoStreamStepByStep() throws Exception {
+        String url = Http2TestServer.getEchoStreamUrl();
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        String[] testData = {"a", "bb", "ccc", "Test String", "1234567890", "woot!"};
+        StringBuilder stringData = new StringBuilder();
+        for (String writeData : testData) {
+            callback.addWriteData(writeData.getBytes());
+            stringData.append(writeData);
+        }
+        // Create stream.
+        BidirectionalStream stream =
+                new BidirectionalStream.Builder(url, callback, callback.getExecutor(),
+                                               mTestFramework.mCronetEngine)
+                        .build();
+        stream.start();
+        callback.waitForNextWriteStep();
+        callback.waitForNextReadStep();
+
+        for (String expected : testData) {
+            // Write next chunk of test data.
+            callback.startNextWrite(stream);
+            callback.waitForNextWriteStep();
+
+            // Read next chunk of test data.
+            ByteBuffer readBuffer = ByteBuffer.allocateDirect(100);
+            callback.startNextRead(stream, readBuffer);
+            callback.waitForNextReadStep();
+            assertEquals(expected.length(), readBuffer.position());
+            assertFalse(stream.isDone());
+        }
+
+        callback.setAutoAdvance(true);
+        callback.startNextRead(stream);
+        callback.blockForDone();
+        assertTrue(stream.isDone());
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals(stringData.toString(), callback.mResponseAsString);
+    }
+
+    /**
+     * Checks that the buffer is updated correctly, when starting at an offset.
+     */
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testSimpleGetBufferUpdates() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        // Since the method is "GET", the expected response body is also "GET".
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        BidirectionalStream stream = builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.waitForNextReadStep();
+
+        assertEquals(null, callback.mError);
+        assertFalse(callback.isDone());
+        assertEquals(TestBidirectionalStreamCallback.ResponseStep.ON_RESPONSE_STARTED,
+                callback.mResponseStep);
+
+        ByteBuffer readBuffer = ByteBuffer.allocateDirect(5);
+        readBuffer.put("FOR".getBytes());
+        assertEquals(3, readBuffer.position());
+
+        // Read first two characters of the response ("GE"). It's theoretically
+        // possible to need one read per character, though in practice,
+        // shouldn't happen.
+        while (callback.mResponseAsString.length() < 2) {
+            assertFalse(callback.isDone());
+            callback.startNextRead(stream, readBuffer);
+            callback.waitForNextReadStep();
+        }
+
+        // Make sure the two characters were read.
+        assertEquals("GE", callback.mResponseAsString);
+
+        // Check the contents of the entire buffer. The first 3 characters
+        // should not have been changed, and the last two should be the first
+        // two characters from the response.
+        assertEquals("FORGE", bufferContentsToString(readBuffer, 0, 5));
+        // The limit and position should be 5.
+        assertEquals(5, readBuffer.limit());
+        assertEquals(5, readBuffer.position());
+
+        assertEquals(ResponseStep.ON_READ_COMPLETED, callback.mResponseStep);
+
+        // Start reading from position 3. Since the only remaining character
+        // from the response is a "T", when the read completes, the buffer
+        // should contain "FORTE", with a position() of 4 and a limit() of 5.
+        readBuffer.position(3);
+        callback.startNextRead(stream, readBuffer);
+        callback.waitForNextReadStep();
+
+        // Make sure all three characters of the response have now been read.
+        assertEquals("GET", callback.mResponseAsString);
+
+        // Check the entire contents of the buffer. Only the third character
+        // should have been modified.
+        assertEquals("FORTE", bufferContentsToString(readBuffer, 0, 5));
+
+        // Make sure position and limit were updated correctly.
+        assertEquals(4, readBuffer.position());
+        assertEquals(5, readBuffer.limit());
+
+        assertEquals(ResponseStep.ON_READ_COMPLETED, callback.mResponseStep);
+
+        // One more read attempt. The request should complete.
+        readBuffer.position(1);
+        readBuffer.limit(5);
+        callback.startNextRead(stream, readBuffer);
+        callback.waitForNextReadStep();
+
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("GET", callback.mResponseAsString);
+        checkResponseInfo(callback.mResponseInfo, Http2TestServer.getEchoMethodUrl(), 200, "");
+
+        // Check that buffer contents were not modified.
+        assertEquals("FORTE", bufferContentsToString(readBuffer, 0, 5));
+
+        // Position should not have been modified, since nothing was read.
+        assertEquals(1, readBuffer.position());
+        // Limit should be unchanged as always.
+        assertEquals(5, readBuffer.limit());
+
+        assertEquals(ResponseStep.ON_SUCCEEDED, callback.mResponseStep);
+
+        // Make sure there are no other pending messages, which would trigger
+        // asserts in TestBidirectionalCallback.
+        testSimpleGet();
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testBadBuffers() throws Exception {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        BidirectionalStream stream = builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.waitForNextReadStep();
+
+        assertEquals(null, callback.mError);
+        assertFalse(callback.isDone());
+        assertEquals(TestBidirectionalStreamCallback.ResponseStep.ON_RESPONSE_STARTED,
+                callback.mResponseStep);
+
+        // Try to read using a full buffer.
+        try {
+            ByteBuffer readBuffer = ByteBuffer.allocateDirect(4);
+            readBuffer.put("full".getBytes());
+            stream.read(readBuffer);
+            fail("Exception not thrown");
+        } catch (IllegalArgumentException e) {
+            assertEquals("ByteBuffer is already full.", e.getMessage());
+        }
+
+        // Try to read using a non-direct buffer.
+        try {
+            ByteBuffer readBuffer = ByteBuffer.allocate(5);
+            stream.read(readBuffer);
+            fail("Exception not thrown");
+        } catch (Exception e) {
+            assertEquals("byteBuffer must be a direct ByteBuffer.", e.getMessage());
+        }
+
+        // Finish the stream with a direct ByteBuffer.
+        callback.setAutoAdvance(true);
+        ByteBuffer readBuffer = ByteBuffer.allocateDirect(5);
+        stream.read(readBuffer);
+        callback.blockForDone();
+        assertEquals(200, callback.mResponseInfo.getHttpStatusCode());
+        assertEquals("GET", callback.mResponseAsString);
+    }
+
+    private void throwOrCancel(FailureType failureType, ResponseStep failureStep,
+            boolean expectResponseInfo, boolean expectError) {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setFailure(failureType, failureStep);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        BidirectionalStream stream = builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.blockForDone();
+        // assertEquals(callback.mResponseStep, failureStep);
+        assertTrue(stream.isDone());
+        assertEquals(expectResponseInfo, callback.mResponseInfo != null);
+        assertEquals(expectError, callback.mError != null);
+        assertEquals(expectError, callback.mOnErrorCalled);
+        assertEquals(failureType == FailureType.CANCEL_SYNC
+                        || failureType == FailureType.CANCEL_ASYNC
+                        || failureType == FailureType.CANCEL_ASYNC_WITHOUT_PAUSE,
+                callback.mOnCanceledCalled);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testFailures() throws Exception {
+        throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_REQUEST_HEADERS_SENT, false, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_REQUEST_HEADERS_SENT, false, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_REQUEST_HEADERS_SENT,
+                false, false);
+        throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_REQUEST_HEADERS_SENT, false, true);
+
+        throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_RESPONSE_STARTED, true, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_RESPONSE_STARTED, true, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_RESPONSE_STARTED,
+                true, false);
+        throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_RESPONSE_STARTED, true, true);
+
+        throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_READ_COMPLETED, true, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_READ_COMPLETED, true, false);
+        throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_READ_COMPLETED, true,
+                false);
+        throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_READ_COMPLETED, true, true);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testThrowOnSucceeded() {
+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setFailure(FailureType.THROW_SYNC, ResponseStep.ON_SUCCEEDED);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        BidirectionalStream stream = builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.blockForDone();
+        assertEquals(callback.mResponseStep, ResponseStep.ON_SUCCEEDED);
+        assertTrue(stream.isDone());
+        assertNotNull(callback.mResponseInfo);
+        // Check that error thrown from 'onSucceeded' callback is not reported.
+        assertNull(callback.mError);
+        assertFalse(callback.mOnErrorCalled);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testExecutorShutdownBeforeStreamIsDone() {
+        // Test that stream is destroyed even if executor is shutdown and rejects posting tasks.

[xunjieli, 2016/01/27 16:16:38]

nit: s/shutdown/shut down

[mef, 2016/01/27 19:06:47]

Done.

+        TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback();
+        callback.setAutoAdvance(false);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        CronetBidirectionalStream stream =
+                (CronetBidirectionalStream) builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.waitForNextReadStep();
+        assertFalse(callback.isDone());
+        assertFalse(stream.isDone());
+
+        final ConditionVariable streamDestroyed = new ConditionVariable(false);
+        stream.setOnDestroyedCallbackForTesting(new Runnable() {
+            @Override
+            public void run() {
+                streamDestroyed.open();
+            }
+        });
+
+        // Shut down the executor, so posting the task will throw an exception.
+        callback.shutdownExecutor();
+        ByteBuffer readBuffer = ByteBuffer.allocateDirect(5);
+        stream.read(readBuffer);
+        // Callback will never be called again because executor is shut down,
+        // but stream will be destroyed from network thread.
+        streamDestroyed.block();
+
+        assertFalse(callback.isDone());
+        assertTrue(stream.isDone());
+    }
+
+    /**
+     * Callback that shuts down the engine when the stream has succeeded
+     * or failed.
+     */
+    private class ShutdownTestBidirectionalStreamCallback extends TestBidirectionalStreamCallback {
+        @Override
+        public void onSucceeded(BidirectionalStream stream, UrlResponseInfo info) {
+            mTestFramework.mCronetEngine.shutdown();
+            // Clear mCronetEngine so it doesn't get shut down second time in tearDown().
+            mTestFramework.mCronetEngine = null;
+            super.onSucceeded(stream, info);
+        }
+
+        @Override
+        public void onFailed(
+                BidirectionalStream stream, UrlResponseInfo info, CronetException error) {
+            mTestFramework.mCronetEngine.shutdown();
+            // Clear mCronetEngine so it doesn't get shut down second time in tearDown().
+            mTestFramework.mCronetEngine = null;
+            super.onFailed(stream, info, error);
+        }
+
+        @Override
+        public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) {
+            mTestFramework.mCronetEngine.shutdown();
+            // Clear mCronetEngine so it doesn't get shut down second time in tearDown().
+            mTestFramework.mCronetEngine = null;
+            super.onCanceled(stream, info);
+        }
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testCronetEngineShutdown() throws Exception {
+        // Test that CronetEngine cannot be shut down if there are any active streams.
+        TestBidirectionalStreamCallback callback = new ShutdownTestBidirectionalStreamCallback();
+        // Block callback when response starts to verify that shutdown fails
+        // if there are active streams.
+        callback.setAutoAdvance(false);
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        CronetBidirectionalStream stream =
+                (CronetBidirectionalStream) builder.setHttpMethod("GET").build();
+        stream.start();
+        try {
+            mTestFramework.mCronetEngine.shutdown();
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Cannot shutdown with active requests.", e.getMessage());

[xunjieli, 2016/01/27 16:16:38]

nit: s/shutdown/shut down, or "Cannot call shutdown".

[mef, 2016/01/27 19:06:47]

That's the exception message,  untouched by this CL.

+        }
+
+        callback.waitForNextReadStep();
+        assertEquals(ResponseStep.ON_RESPONSE_STARTED, callback.mResponseStep);
+        try {
+            mTestFramework.mCronetEngine.shutdown();
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Cannot shutdown with active requests.", e.getMessage());
+        }
+        callback.startNextRead(stream);
+
+        callback.waitForNextReadStep();
+        assertEquals(ResponseStep.ON_READ_COMPLETED, callback.mResponseStep);
+        try {
+            mTestFramework.mCronetEngine.shutdown();
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Cannot shutdown with active requests.", e.getMessage());
+        }
+
+        // May not have read all the data, in theory. Just enable auto-advance
+        // and finish the request.
+        callback.setAutoAdvance(true);
+        callback.startNextRead(stream);
+        callback.blockForDone();
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testCronetEngineShutdownAfterStreamFailure() throws Exception {
+        // Test that CronetEngine can be shut down after stream reports a failure.
+        TestBidirectionalStreamCallback callback = new ShutdownTestBidirectionalStreamCallback();
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        CronetBidirectionalStream stream =
+                (CronetBidirectionalStream) builder.setHttpMethod("GET").build();
+        stream.start();
+        callback.setFailure(FailureType.THROW_SYNC, ResponseStep.ON_READ_COMPLETED);
+        callback.blockForDone();
+        assertTrue(callback.mOnErrorCalled);
+    }
+
+    @SmallTest
+    @Feature({"Cronet"})
+    @OnlyRunNativeCronet
+    public void testCronetEngineShutdownAfterStreamCancel() throws Exception {
+        // Test that CronetEngine can be shut down after stream is canceled.
+        TestBidirectionalStreamCallback callback = new ShutdownTestBidirectionalStreamCallback();
+        BidirectionalStream.Builder builder =
+                new BidirectionalStream.Builder(Http2TestServer.getEchoMethodUrl(), callback,
+                        callback.getExecutor(), mTestFramework.mCronetEngine);
+        CronetBidirectionalStream stream =
+                (CronetBidirectionalStream) builder.setHttpMethod("GET").build();
+
+        // Block callback when response starts to verify that shutdown fails
+        // if there are active requests.
+        callback.setAutoAdvance(false);
+        stream.start();
+        try {
+            mTestFramework.mCronetEngine.shutdown();
+            fail("Should throw an exception");
+        } catch (Exception e) {
+            assertEquals("Cannot shutdown with active requests.", e.getMessage());
+        }
+        callback.waitForNextReadStep();
+        assertEquals(ResponseStep.ON_RESPONSE_STARTED, callback.mResponseStep);
+        stream.cancel();

[xunjieli, 2016/01/27 16:16:38]

testCronetEngineShutdownAfterStreamFailure and
testCronetEngineShutdownAfterStreamCancel are hard to reason about. I guess your
goal here is to make sure that when the active stream is failed or canceled, we
can shut down the engine. However, the shutdown logic is buried in the onFailed
and onCanceled callback, so it is hard to see.

Suggest adding a AssertNull(mTestFramework.mCronetEngine) at the end of these
two tests to make sure the corresponding callback is invoked.

[mef, 2016/01/27 19:06:47]

Done.

+    }
+
+    // Returns the contents of byteBuffer, from its position() to its limit(),
+    // as a String. Does not modify byteBuffer's position().
+    private static String bufferContentsToString(ByteBuffer byteBuffer, int start, int end) {
+        // Use a duplicate to avoid modifying byteBuffer.
+        ByteBuffer duplicate = byteBuffer.duplicate();
+        duplicate.position(start);
+        duplicate.limit(end);
+        byte[] contents = new byte[duplicate.remaining()];
+        duplicate.get(contents);
+        return new String(contents);
     }
 }