[Cronet] JavaUrlRequest - Don't call close() on a user thread - it can do I/O.

Don't call close() on a user thread - it can do I/O.

Committed: https://crrev.com/260cd265e1ab81c875d573a8d9736af8662de85b
Cr-Commit-Position: refs/heads/master@{#414843}

[Charles, 2016-08-25 20:46:38]

clm@google.com changed reviewers:
+ mef@chromium.org

[mef, 2016-08-25 22:23:54]

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802:
closeResponseChannel();
would it make sense to just call closeQuietly from the runnable below (similarly
in onFailed)?

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

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:852:
urlRequest.cancel();
Do you need to setup something to trigger IO on cancel?

[Charles, 2016-08-26 15:58:19]

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802:
closeResponseChannel();
On 2016/08/25 22:23:54, mef wrote:
> would it make sense to just call closeQuietly from the runnable below
(similarly
> in onFailed)?

No, because then we'd be doing I/O on the user's executor rather than our own.

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

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:852:
urlRequest.cancel();
On 2016/08/25 22:23:54, mef wrote:
> Do you need to setup something to trigger IO on cancel?

I do set up - this is in the middle of a request when I cancel.

[mef, 2016-08-26 16:14:11]

lgtm

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:659:
mResponseChannel.close();
Do we need to closeResponseChannel() here as well?

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802:
closeResponseChannel();
On 2016/08/26 15:58:19, Charles wrote:
> On 2016/08/25 22:23:54, mef wrote:
> > would it make sense to just call closeQuietly from the runnable below
> (similarly
> > in onFailed)?
> 
> No, because then we'd be doing I/O on the user's executor rather than our own.


Oh, I see. I've missed that those are different executors.

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:849:
mExecutor.execute(new Runnable() {
what if mExecutor rejects execution?

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

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:852:
urlRequest.cancel();
On 2016/08/26 15:58:19, Charles wrote:
> On 2016/08/25 22:23:54, mef wrote:
> > Do you need to setup something to trigger IO on cancel?
> 
> I do set up - this is in the middle of a request when I cancel. 

Acknowledged.

[Charles, 2016-08-26 18:15:51]

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:659:
mResponseChannel.close();
On 2016/08/26 16:14:11, mef wrote:
> Do we need to closeResponseChannel() here as well?

No, processReadResult is always called on mExecutor, and we want to capture any
exceptions thrown here as I/O errors.

https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:849:
mExecutor.execute(new Runnable() {
On 2016/08/26 16:14:11, mef wrote:
> what if mExecutor rejects execution?

It can't, we use an unbounded thread pool that's never shut down for our I/O.
That's why usage of mExecutor doesn't check REE.

[mef, 2016-08-26 18:43:57]

On 2016/08/26 18:15:51, Charles wrote:
>
https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
> File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
> (right):
> 
>
https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
> components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:659:
> mResponseChannel.close();
> On 2016/08/26 16:14:11, mef wrote:
> > Do we need to closeResponseChannel() here as well?
> 
> No, processReadResult is always called on mExecutor, and we want to capture
any
> exceptions thrown here as I/O errors.
> 
>
https://codereview.chromium.org/2283513002/diff/1/components/cronet/android/a...
> components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:849:
> mExecutor.execute(new Runnable() {
> On 2016/08/26 16:14:11, mef wrote:
> > what if mExecutor rejects execution?
> 
> It can't, we use an unbounded thread pool that's never shut down for our I/O.
> That's why usage of mExecutor doesn't check REE.

Thanks for explanations, still lgtm

[Charles, 2016-08-26 21:57:49]

The CQ bit was checked by clm@google.com

[commit-bot: I haz the power, 2016-08-26 21:58:23]

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-08-26 22:47:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-26 22:51:51]

Description was changed from

==========
Don't call close() on a user thread - it can do I/O.
==========

to

==========
Don't call close() on a user thread - it can do I/O.

Committed: https://crrev.com/260cd265e1ab81c875d573a8d9736af8662de85b
Cr-Commit-Position: refs/heads/master@{#414843}
==========

[commit-bot: I haz the power, 2016-08-26 22:51:52]

Patchset 1 (id:??) landed as
https://crrev.com/260cd265e1ab81c875d573a8d9736af8662de85b
Cr-Commit-Position: refs/heads/master@{#414843}