Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2283243002:
Allow direct executors in cronet. (Closed)
DescriptionAllow direct executors in cronet.
Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads.
BUG=566195
Committed: https://crrev.com/813945b2126b168e154d0f54c70bae7e77f05add
Cr-Commit-Position: refs/heads/master@{#416287}
Patch Set 1 #
Total comments: 45
Patch Set 2 : Addressed comments #
Total comments: 1
Patch Set 3 : Addressed comments, unified request failure logic in CronetUrlRequest #
Total comments: 4
Patch Set 4 : Refactor DirectPreventingExecutor, and fix closing logic #
Total comments: 6
Patch Set 5 : Added test for context shutdown. #Patch Set 6 : Sync and rebase #Patch Set 7 : Improve thread checking #
Total comments: 2
Messages
Total messages: 29 (8 generated)
Description was changed from ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necesarily need to hop back and forth between threads. BUG=566195 ========== to ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necesarily need to hop back and forth between threads. BUG=566195 ==========
clm@google.com changed reviewers: + mef@chromium.org
Description was changed from ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necesarily need to hop back and forth between threads. BUG=566195 ========== to ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads. BUG=566195 ==========
This is part one, to be continued. :) https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:859: boolean disableCache, boolean disableConnectionMigration, boolean allowDirectExecutor); G'Ah. This will break all the HackMockCronetEngines that override this. :( https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java:49: mExecutedInline = new InlineExecutionProhibitedException(); Can it just throw from here instead of returning? It is on caller thread, so it should just work. If you covering the case where mDelegate catches exceptions, maybe add a comment? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java:15: super("Inline execution is prohibited for this request"); Is there java / google guide on whether or not exception messages should end with period? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:125: this.mAllowDirectExecutor = allowDirectExecutor; nit: move it to the end, below userAgent? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:368: mUserExecutor.execute(uploadErrorSetting(runnable)); Should this execute on mUploadExecutor instead of mUserExecutor? We probably need a companion method to executeOnUserExecutor(). https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802: if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) { This is changing thread where mState is accessed from executor. Why is it needed and why is it safe? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:813: if (mState.compareAndSet(State.READING, State.AWAITING_READ)) { ditto. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequest.java:276: final UrlRequest request = mCronetEngine.createRequest(mUrl, mCallback, mExecutor, Can this executor be also just wrapped into DirectPreventingExecutor here instead of passing flag down? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_adapter.cc:139: DCHECK(!context_->IsOnNetworkThread()); Remove this? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:313: postTaskToExecutor(new Runnable() { this is already on executor thread, why do you need to post?
https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: failWithException( I think this could cause destroyRequestAdapter() to be called twice - once here, and once in task posted in failedWithException(). https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:762: throw new InlineExecutionProhibitedException(); I think this will cause native crash - exception will be thrown on the network thread and jni wrappers will CHECK as it is not handled. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (left): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:495: synchronized (mLock) { what's wrong with synchronized here? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:629: public boolean isNativeThread(Thread thread) { maybe call it isNetworkThread()? Also maybe get current thread here instead of passing in? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:114: public void setAllowDirectExecutor(boolean allowed) { maybe instead of setAllowDirectExecutor() use setUseDirectExecutor() and have checks below verify that callbacks are done on network thread? https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:252: System.out.println("opened"); nit: remove
PTAL https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:859: boolean disableCache, boolean disableConnectionMigration, boolean allowDirectExecutor); On 2016/08/30 16:29:44, mef wrote: > G'Ah. This will break all the HackMockCronetEngines that override this. :( Acknowledged. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java:49: mExecutedInline = new InlineExecutionProhibitedException(); On 2016/08/30 16:29:44, mef wrote: > Can it just throw from here instead of returning? > It is on caller thread, so it should just work. > If you covering the case where mDelegate catches exceptions, maybe add a > comment? Done. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java:15: super("Inline execution is prohibited for this request"); On 2016/08/30 16:29:44, mef wrote: > Is there java / google guide on whether or not exception messages should end > with period? There's no official guidance, but the java core libraries team doesn't add a period at the end of exception messages they construct. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:125: this.mAllowDirectExecutor = allowDirectExecutor; On 2016/08/30 16:29:44, mef wrote: > nit: move it to the end, below userAgent? This value gets read by AsyncUrlRequestCallback. Moved there. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:368: mUserExecutor.execute(uploadErrorSetting(runnable)); On 2016/08/30 16:29:44, mef wrote: > Should this execute on mUploadExecutor instead of mUserExecutor? > We probably need a companion method to executeOnUserExecutor(). In this lexical scope mUserExecutor is referring to the mUserExecutor of OutputStreamDataSink, which is the upload executor. There is no field for the user executor except in AsyncUrlRequestCallback. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802: if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) { On 2016/08/30 16:29:44, mef wrote: > This is changing thread where mState is accessed from executor. > Why is it needed and why is it safe? mState is safe from any thread, since it's an atomic reference. This transition is necessary to handle rejected execution exceptions properly, since our state transition relies on knowing what the state is before we invoke something that could trigger a user error. This code is now handling https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:813: if (mState.compareAndSet(State.READING, State.AWAITING_READ)) { On 2016/08/30 16:29:44, mef wrote: > ditto. This one too - we want a REE thrown when submitting to count as a user failure, and we want to be in the correct state before calling execute(). https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequest.java:276: final UrlRequest request = mCronetEngine.createRequest(mUrl, mCallback, mExecutor, On 2016/08/30 16:29:44, mef wrote: > Can this executor be also just wrapped into DirectPreventingExecutor here > instead of passing flag down? I didn't do that in this case because we pool the runnables that we're submitting to the executor for reads here, and the checking for the native impl is much simpler (just a check if we're on the network thread). https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_adapter.cc:139: DCHECK(!context_->IsOnNetworkThread()); On 2016/08/30 16:29:44, mef wrote: > Remove this? Done. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:313: postTaskToExecutor(new Runnable() { On 2016/08/30 16:29:45, mef wrote: > this is already on executor thread, why do you need to post? It's not on the executor thread any more. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: failWithException( On 2016/08/30 17:55:47, mef wrote: > I think this could cause destroyRequestAdapter() to be called twice - once here, > and once in task posted in failedWithException(). That's OK, destroyRequestAdapter() is idempotent. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:762: throw new InlineExecutionProhibitedException(); On 2016/08/30 17:55:47, mef wrote: > I think this will cause native crash - exception will be thrown on the network > thread and jni wrappers will CHECK as it is not handled. The exception is on the network thread, but it'll get caught in postTaskToExecutor() and so it won't make it into native code. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (left): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:495: synchronized (mLock) { On 2016/08/30 17:55:47, mef wrote: > what's wrong with synchronized here? Nothing, it's just unnecessary. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:629: public boolean isNativeThread(Thread thread) { On 2016/08/30 17:55:47, mef wrote: > maybe call it isNetworkThread()? Also maybe get current thread here instead of > passing in? Renamed. The API is more flexible if I allow it to be passed in though - avoids multiple unnecessary calls to Thread.currentThread(). https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:114: public void setAllowDirectExecutor(boolean allowed) { On 2016/08/30 17:55:47, mef wrote: > maybe instead of setAllowDirectExecutor() use setUseDirectExecutor() and have > checks below verify that callbacks are done on network thread? That wouldn't fly for javaurlrequest, which has a thread pool it uses for networking operations. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:252: System.out.println("opened"); On 2016/08/30 17:55:47, mef wrote: > nit: remove Done.
https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java:15: super("Inline execution is prohibited for this request"); On 2016/08/30 20:11:05, Charles wrote: > On 2016/08/30 16:29:44, mef wrote: > > Is there java / google guide on whether or not exception messages should end > > with period? > > There's no official guidance, but the java core libraries team doesn't add a > period at the end of exception messages they construct. Acknowledged. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:368: mUserExecutor.execute(uploadErrorSetting(runnable)); On 2016/08/30 20:11:05, Charles wrote: > On 2016/08/30 16:29:44, mef wrote: > > Should this execute on mUploadExecutor instead of mUserExecutor? > > We probably need a companion method to executeOnUserExecutor(). > > In this lexical scope mUserExecutor is referring to the mUserExecutor of > OutputStreamDataSink, which is the upload executor. There is no field for the > user executor except in AsyncUrlRequestCallback. I see. I've missed the fact that this is part of OutputStreamDataSink and not JavaUrlRequest. It is still mildly confusing that executeOnUploadExecutor is running tasks on |mUserExecutor|. Maybe rename mUserExecutor -> mUserUploadExecutor or mUploadExecutor? I don't feel strongly though. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:802: if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) { On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 16:29:44, mef wrote: > > This is changing thread where mState is accessed from executor. > > Why is it needed and why is it safe? > > mState is safe from any thread, since it's an atomic reference. This transition > is necessary to handle rejected execution exceptions properly, since our state > transition relies on knowing what the state is before we invoke something that > could trigger a user error. This code is now handling Acknowledged. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequest.java:276: final UrlRequest request = mCronetEngine.createRequest(mUrl, mCallback, mExecutor, On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 16:29:44, mef wrote: > > Can this executor be also just wrapped into DirectPreventingExecutor here > > instead of passing flag down? > > I didn't do that in this case because we pool the runnables that we're > submitting to the executor for reads here, and the checking for the native impl > is much simpler (just a check if we're on the network thread). Acknowledged. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:313: postTaskToExecutor(new Runnable() { On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 16:29:45, mef wrote: > > this is already on executor thread, why do you need to post? > > It's not on the executor thread any more. Acknowledged. Please update comment. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: failWithException( On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 17:55:47, mef wrote: > > I think this could cause destroyRequestAdapter() to be called twice - once > here, > > and once in task posted in failedWithException(). > > That's OK, destroyRequestAdapter() is idempotent. Fair point. Could you update the comment above? I'm also missing what scenario does failWithException address here, given that its runnable is just as likely to be rejected as this |task|. https://codereview.chromium.org/2283243002/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java (right): https://codereview.chromium.org/2283243002/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/DirectPreventingExecutor.java:49: // exception nit: Comments in Chrome must actually end with period.
looks good, just one more nit. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:762: throw new InlineExecutionProhibitedException(); On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 17:55:47, mef wrote: > > I think this will cause native crash - exception will be thrown on the network > > thread and jni wrappers will CHECK as it is not handled. > > The exception is on the network thread, but it'll get caught in > postTaskToExecutor() and so it won't make it into native code. Ah, good point. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (left): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:495: synchronized (mLock) { On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 17:55:47, mef wrote: > > what's wrong with synchronized here? > > Nothing, it's just unnecessary. Acknowledged. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:629: public boolean isNativeThread(Thread thread) { On 2016/08/30 20:11:06, Charles wrote: > On 2016/08/30 17:55:47, mef wrote: > > maybe call it isNetworkThread()? Also maybe get current thread here instead of > > passing in? > > Renamed. Um, that didn't work? > The API is more flexible if I allow it to be passed in though - avoids > multiple unnecessary calls to Thread.currentThread(). Ack, sgtm.
PTAL https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java:368: mUserExecutor.execute(uploadErrorSetting(runnable)); On 2016/08/31 17:26:52, mef wrote: > On 2016/08/30 20:11:05, Charles wrote: > > On 2016/08/30 16:29:44, mef wrote: > > > Should this execute on mUploadExecutor instead of mUserExecutor? > > > We probably need a companion method to executeOnUserExecutor(). > > > > In this lexical scope mUserExecutor is referring to the mUserExecutor of > > OutputStreamDataSink, which is the upload executor. There is no field for the > > user executor except in AsyncUrlRequestCallback. > > I see. I've missed the fact that this is part of OutputStreamDataSink and not > JavaUrlRequest. It is still mildly confusing that executeOnUploadExecutor is > running tasks on |mUserExecutor|. Maybe rename mUserExecutor -> > mUserUploadExecutor or mUploadExecutor? I don't feel strongly though. Done. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java:313: postTaskToExecutor(new Runnable() { On 2016/08/31 17:26:52, mef wrote: > On 2016/08/30 20:11:06, Charles wrote: > > On 2016/08/30 16:29:45, mef wrote: > > > this is already on executor thread, why do you need to post? > > > > It's not on the executor thread any more. > > Acknowledged. Please update comment. Done. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: failWithException( On 2016/08/31 17:26:52, mef wrote: > On 2016/08/30 20:11:06, Charles wrote: > > On 2016/08/30 17:55:47, mef wrote: > > > I think this could cause destroyRequestAdapter() to be called twice - once > > here, > > > and once in task posted in failedWithException(). > > > > That's OK, destroyRequestAdapter() is idempotent. > > Fair point. Could you update the comment above? > I'm also missing what scenario does failWithException address here, given that > its runnable is just as likely to be rejected as this |task|. Comment updated. I wouldn't say it's "just as likely" - there's circumstances where it is worth trying. https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:629: public boolean isNativeThread(Thread thread) { On 2016/08/31 18:30:11, mef wrote: > On 2016/08/30 20:11:06, Charles wrote: > > On 2016/08/30 17:55:47, mef wrote: > > > maybe call it isNetworkThread()? Also maybe get current thread here instead > of > > > passing in? > > > > Renamed. > Um, that didn't work? > > > The API is more flexible if I allow it to be passed in though - avoids > > multiple unnecessary calls to Thread.currentThread(). > Ack, sgtm. > Done.
couple more nits. https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:375: // direct execution not permitted), or caused the the runnables we submit if nit: the the -> by the? https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: // latter two cases, we at least have a chance to inform the embedder of the request's nit: We don't use 'we' in Chromium comments. :)
All tests passing! https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:375: // direct execution not permitted), or caused the the runnables we submit if On 2016/08/31 20:24:25, mef wrote: > nit: the the -> by the? Done. https://codereview.chromium.org/2283243002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:377: // latter two cases, we at least have a chance to inform the embedder of the request's On 2016/08/31 20:24:25, mef wrote: > nit: We don't use 'we' in Chromium comments. :) Done.
Looks much cleaner now! https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java:13: public final class InlineExecutionProhibitedException extends RejectedExecutionException { nit: Maybe call it DirectExecutionProhibitedException to match AllowDirectExecutor flag? https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:478: private void checkHaveAdapter() throws IllegalStateException { We use naming convention XyzLocked in CronetUrlRequest for methods guarded by the lock (for example isDoneLocked()). Maybe we should do the same here and below? https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1418: public void testDirectExecutorAllowed() throws Exception { Would it make sense to add a test that canceling request from a callback invoked on direct executor works as expected? Also, it would be useful to verify that CronetEngine.shutdown() from request completion callback on direct executor throws an exception as expected?
Test added. https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/InlineExecutionProhibitedException.java:13: public final class InlineExecutionProhibitedException extends RejectedExecutionException { On 2016/09/01 15:17:18, mef wrote: > nit: Maybe call it DirectExecutionProhibitedException to match > AllowDirectExecutor flag? I named it this because people encountering this exception aren't likely to know what a direct executor is - they'll probably hit this by writing one themselves, or using caller_runs_policy. https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:478: private void checkHaveAdapter() throws IllegalStateException { On 2016/09/01 15:17:18, mef wrote: > We use naming convention XyzLocked in CronetUrlRequest for methods guarded by > the lock (for example isDoneLocked()). > Maybe we should do the same here and below? But why? The compiler checks your locking once you've annotated with @GuardedBy. https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2283243002/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:1418: public void testDirectExecutorAllowed() throws Exception { On 2016/09/01 15:17:18, mef wrote: > Would it make sense to add a test that canceling request from a callback invoked > on direct executor works as expected? > Also, it would be useful to verify that CronetEngine.shutdown() from request > completion callback on direct executor throws an exception as expected? Done.
lgtm, thanks a lot!
The CQ bit was checked by clm@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/09/01 18:43:01, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) > android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) > cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) > chromeos_amd64-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) > chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) > chromeos_x86-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) still lgtm
Ah, nice! https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:245: checkExecutorThread(); Can this be just if (!error.getCause() instanceof InlineExecutionProhibitedException) { checkExecutorThread(); } without changing mAllowDirectExecutor?
https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java (right): https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestCallback.java:245: checkExecutorThread(); On 2016/09/02 15:01:47, mef wrote: > Can this be just > > if (!error.getCause() instanceof InlineExecutionProhibitedException) { > checkExecutorThread(); > } > > without changing mAllowDirectExecutor? No, because maybeThrowCancelOrPause() also calls checkExecutorThread.
Thanks for explanation, lgtm
The CQ bit was checked by clm@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads. BUG=566195 ========== to ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads. BUG=566195 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads. BUG=566195 ========== to ========== Allow direct executors in cronet. Direct executors are banned by default - if cronet detects an executor with this behavior, it will fail the request immediately. Callers can disable this enforcement by using a new method on UrlRequest.Builder. Callers can use directexecutor to get finer grained control over their threading behavior - lightweight callbacks won't necessarily need to hop back and forth between threads. BUG=566195 Committed: https://crrev.com/813945b2126b168e154d0f54c70bae7e77f05add Cr-Commit-Position: refs/heads/master@{#416287} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/813945b2126b168e154d0f54c70bae7e77f05add Cr-Commit-Position: refs/heads/master@{#416287} |
