[Cronet] Mark request as complete when OutputStream fails

[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.

R=pauljensen@chromium.org
BUG=630664
Committed: https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16
Cr-Commit-Position: refs/heads/master@{#407468}

[xunjieli, 2016-07-22 17:39:04]

Paul, PTAL, thanks! I will manually test this on the client app too, and will
update this thread when I get the result.

[xunjieli, 2016-07-22 17:40:38]

Description was changed from

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljense@chromium.org
BUG=630664
==========

to

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljense@chromium.org
BUG=630664
==========

[xunjieli, 2016-07-22 17:40:38]

xunjieli@chromium.org changed reviewers:
+ pauljensen@chromium.org
- pauljense@chromium.org

[xunjieli, 2016-07-22 17:40:48]

Description was changed from

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljense@chromium.org
BUG=630664
==========

to

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljensen@chromium.org
BUG=630664
==========

[xunjieli, 2016-07-22 18:40:14]

On 2016/07/22 17:39:04, xunjieli wrote:
> Paul, PTAL, thanks! I will manually test this on the client app too, and will
> update this thread when I get the result.

Paul, I tested client manually, and Cronet no longer blocks the calling thread
with this change. But it appears there is something else going on in their app
that makes upload hang. I will keep an eye on it.

[pauljensen, 2016-07-22 18:54:29]

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
(right):

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:110:
} catch (IOException e) {
should there be a fail() after out.write(1)?

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:121:
} catch (IOException e) {
should there be a fail() after connection getResponseCode()?

[xunjieli, 2016-07-22 20:01:09]

Great catch! I found that the first fail() doesn't work correctly for Cronet
because once the OutputStream exits loop() and if it doesn't try upload more, it
won't know if there is an exception. I also add an equivalent test for chunked
upload. 

PTAL. Thanks!

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
(right):

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:110:
} catch (IOException e) {
On 2016/07/22 18:54:29, pauljensen wrote:
> should there be a fail() after out.write(1)?

Done.

https://codereview.chromium.org/2173923002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:121:
} catch (IOException e) {
On 2016/07/22 18:54:29, pauljensen wrote:
> should there be a fail() after connection getResponseCode()?

Done.

[xunjieli, 2016-07-22 20:15:24]

Also there is a bit of code duplication between chunked upload and fixed mode
upload. I will try address them in follow-ups.

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

https://codereview.chromium.org/2173923002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:512:
mHasResponse = true;
Without this line, both tests will get stuck at getResponseCode().

[pauljensen, 2016-07-25 11:43:31]

lgtm

[xunjieli, 2016-07-25 13:27:18]

On 2016/07/25 11:43:31, pauljensen wrote:
> lgtm

Thanks for the review!

[xunjieli, 2016-07-25 13:27:23]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-07-25 13:27:42]

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-25 14:00:07]

Description was changed from

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljensen@chromium.org
BUG=630664
==========

to

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljensen@chromium.org
BUG=630664
==========

[commit-bot: I haz the power, 2016-07-25 14:00:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-25 14:01:36]

Description was changed from

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.


R=pauljensen@chromium.org
BUG=630664
==========

to

==========
[Cronet] Mark request as complete when OutputStream fails

When consumer specifies a chunked upload or a fixed stream upload, Cronet's
HttpURLConnection starts the request right away. If an error happens, we will
surface that error as an IOException to consumer. However, if the consumer
tries to call any other API (like getResponseCode(), getResponseMessage())
afterwards, we will spin up the message loop and wait forever.

This CL marks the request as complete if an error occurred in upload, so
getResponseCode() etc. do not spin up the message loop to wait for response.

R=pauljensen@chromium.org
BUG=630664

Committed: https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16
Cr-Commit-Position: refs/heads/master@{#407468}
==========

[commit-bot: I haz the power, 2016-07-25 14:01:37]

Patchset 2 (id:??) landed as
https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16
Cr-Commit-Position: refs/heads/master@{#407468}