[Cronet] Check whether request is done before spinning up message loop

[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
Committed: https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296
Cr-Commit-Position: refs/heads/master@{#404652}

[xunjieli, 2016-07-08 17:29:36]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-07-08 17:31:06]

Andrei, Misha: Paul usually reviews HttpURLConnection wrapper CL, but since he
is out, could you review this CL? This is blocking our embedder's
experimentation. I am hoping we can get this in next week's release. Thanks!

[xunjieli, 2016-07-08 17:35:34]

Description was changed from

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds regression tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
==========

to

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
==========

[mef, 2016-07-08 17:39:10]

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
(right):

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:74:
super.write(oneByte);
Maybe just explicitly call checkNotClosed() here and elsewhere.

[mef, 2016-07-08 17:42:06]

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:505:
private void setRequestCompleted(IOException exception) {
Why the rename? Can you update the comment?

[xunjieli, 2016-07-08 17:56:26]

Thanks for the review! PTAL.

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
(right):

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:74:
super.write(oneByte);
On 2016/07/08 17:39:10, mef wrote:
> Maybe just explicitly call checkNotClosed() here and elsewhere.

Done.

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
(right):

https://codereview.chromium.org/2131323002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:505:
private void setRequestCompleted(IOException exception) {
On 2016/07/08 17:42:06, mef wrote:
> Why the rename? Can you update the comment?

Done.  I think it is not neccessary to change it, so I've reverted to the old
naming. Thanks!

[xunjieli, 2016-07-08 18:19:09]

Patchset #2 (id:40001) has been deleted

[mef, 2016-07-08 22:19:10]

lgtm mod questions.

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java
(right):

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java:104:
// Expected.
Maybe assert that thrown exception is the one the we expect, or is
platform-specific?

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
(right):

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:85:
// Expected.
Maybe assert that thrown exception is the one the we expect, or is
platform-specific?

[xunjieli, 2016-07-11 13:17:30]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2016-07-11 13:17:36]

Thanks!

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java
(right):

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java:104:
// Expected.
On 2016/07/08 22:19:10, mef wrote:
> Maybe assert that thrown exception is the one the we expect, or is
> platform-specific?

Done.

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
(right):

https://codereview.chromium.org/2131323002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:85:
// Expected.
On 2016/07/08 22:19:10, mef wrote:
> Maybe assert that thrown exception is the one the we expect, or is
> platform-specific?

Done.

[commit-bot: I haz the power, 2016-07-11 13:17:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 13:49:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 13:49:50]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-07-11 14:13:36]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-07-11 14:13:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2131323002/#ps80001
(title: "Address comment")

[commit-bot: I haz the power, 2016-07-11 14:13:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 14:17:26]

Description was changed from

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
==========

to

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
==========

[commit-bot: I haz the power, 2016-07-11 14:17:27]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-07-11 14:18:33]

Description was changed from

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653
==========

to

==========
[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG=626653

Committed: https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296
Cr-Commit-Position: refs/heads/master@{#404652}
==========

[commit-bot: I haz the power, 2016-07-11 14:18:34]

Patchset 3 (id:??) landed as
https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296
Cr-Commit-Position: refs/heads/master@{#404652}