Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
725683002:
[Cronet] Initial implementation of HttpURLConnection (Closed)
Description[Cronet] Initial implementation of HttpURLConnection
This CL adds basic support for setting headers and reading response.
BUG=398997
Committed: https://crrev.com/2a66c58f2d83717e43a50b39f75b3facc24e70f8
Cr-Commit-Position: refs/heads/master@{#306939}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Added a private message loop #
Total comments: 2
Patch Set 3 : Fixed comments in the code #
Total comments: 34
Patch Set 4 : Addressed comments #Patch Set 5 : Allocated and copied byte buffer #Patch Set 6 : Get data only when required #
Total comments: 1
Patch Set 7 : Updated test #
Total comments: 26
Patch Set 8 : Addressed Matt's comments #
Total comments: 31
Patch Set 9 : Modified Tests #Patch Set 10 : Added test fixture to compare with the default implementation #
Total comments: 29
Patch Set 11 : Misha's and Matt's comments #
Total comments: 18
Patch Set 12 : Exception handling #
Total comments: 1
Patch Set 13 : Added a null check #Patch Set 14 : Added throw clause #
Total comments: 6
Patch Set 15 : Addressed Misha's comments #
Total comments: 22
Patch Set 16 : Addressed Matt's comments #
Total comments: 4
Patch Set 17 : Terminate loop if waiting for task has been interrupted #
Total comments: 13
Patch Set 18 : Pass in UrlRequestConfig and make MessageLoop implments Executor #
Total comments: 3
Patch Set 19 : make loop failed in all exceptions #
Total comments: 9
Patch Set 20 : Fix findbug warnings #Messages
Total messages: 65 (8 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
This is an initial CL to get things set up. PTAL. Thanks! https://codereview.chromium.org/725683002/diff/1/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/1/components/cronet.gypi#newco... components/cronet.gypi:243: 'target_name': 'cronet_tests', added this target to access some test util methods (eg. those in UploadTestServer.java) in my diffing tool/app. https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:13: class ByteBufferInputStream extends InputStream { Right now I just wrap around the bytebuffer returned by the first onDataReceived() as my InputStream. Not exactly how I should handle bytebuffers returned by additional onDataReceived() callbacks..
nice! https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:17: public ByteBufferInputStream(ByteBuffer buf) { I don't think you can hold on to mBuf as it is only valid during |OnDataReceived| callback. https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:49: private static class HandlerThreadExecutor implements Executor { Maybe we should consider passing Looper to Async API. https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:120: public final void addRequestProperty(String key, String value) { I guess 'add' should allow multiple headers with the same name and 'set' shouldn't? https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:161: mResponseBody = new ByteBufferInputStream(byteBuffer); Yeah, this is wrong. You need to copy data from |byteBuffer| elsewhere.
https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:120: public final void addRequestProperty(String key, String value) { On 2014/11/13 19:16:17, mef wrote: > I guess 'add' should allow multiple headers with the same name and 'set' > shouldn't? Yes, that's the desired behavior. But due to a limitation in Cronet's Async API (https://code.google.com/p/chromium/issues/detail?id=432719), no matter what we do on the HttpURLConnection side, only one distinct header-value entry will be sent. We will need to fix this after we figured out what to do with Cronet's async APIs (https://code.google.com/p/chromium/issues/detail?id=432719). https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:161: mResponseBody = new ByteBufferInputStream(byteBuffer); On 2014/11/13 19:16:17, mef wrote: > Yeah, this is wrong. You need to copy data from |byteBuffer| elsewhere. Acknowledged. Will do.
https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:194: Looper.loop(); Problem: This is noticeable to the embedder, and can cause tasks to run that they may not expect to do so. Unless the standard HttpURLConnection spins the current Looper, we shouldn't be doing this. For example, suppose one of those tasks starts another HttpURLConnection.... I seem to recall Java or Android has a standard message loop/queue class of some sort that we could just spin up and run until done (Basically like looper, but private, so we only run tasks we post to it ourselves).
On 2014/11/13 19:31:28, mmenke wrote: > https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/725683002/diff/1/components/cronet/android/ja... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:194: > Looper.loop(); > Problem: This is noticeable to the embedder, and can cause tasks to run that > they may not expect to do so. Unless the standard HttpURLConnection spins the > current Looper, we shouldn't be doing this. > > For example, suppose one of those tasks starts another HttpURLConnection.... > > I seem to recall Java or Android has a standard message loop/queue class of some > sort that we could just spin up and run until done (Basically like looper, but > private, so we only run tasks we post to it ourselves). Hrm... I could have sworn I saw a class that seemed to do just what we wanted when looking into some of the details of the Thread class last week. Can't seem to find anything now, though. It shouldn't be too difficult to implement something ourselves, but I'd rather not, if we don't have to.
I have modified my CL to run a private message loop instead of running current thread's looper, and I also made a copy of bytebuffer in onDataReceived (still need to handle multiple onDataReceived() calls, but I will do it in a separate follow-up CL). PTAL. Thanks!
https://codereview.chromium.org/725683002/diff/20001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/20001/components/cronet.gypi#n... components/cronet.gypi:242: { # cronet_tests.jar implements HttpUrlRequest interface using Chromium stack This comment is not true. Will change it in next CL. https://codereview.chromium.org/725683002/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:102: // FIXME: we should send all headers. This is no longer a FIXME, but a TODO. I will add bug number here in next patch for future reference.
Sorry, not going to have a chance to get to this today. I'll do it tomorrow morning.
https://codereview.chromium.org/725683002/diff/40001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet.gypi#n... components/cronet.gypi:243: 'target_name': 'cronet_tests', Why is this needed? https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:14: nit: Remove blank line, to be consistent with other cronet files (Goes for the other Java files, too). https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:15: private final ByteBuffer mBuf; optional nit: Suggest just writing out buffer for readability. It is a common enough abbreviation that the C++ style guide does allow for it, though. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:18: // Makes a read-only copy of the buffer passed in. This is not actually a copy, but a read only reference to the data pointed at by the old buffer. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:41: private final BlockingQueue<MessageLoop.Message> mMessageQueue; nit: Don't think we need a blank line between every pair of variables. Suggest grouping related ones. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:43: nit: Remove extra line break. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: nit: Remove extra blank line. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:57: nit: -blank line. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:153: nit: -blank line https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:166: // TODO(xunjieli): handle streaming. nit: Capitalize https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:173: // TODO(xunjieli): handle redirect. nit: Capitalize https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:185: // TODO(xunjieli): handle failure. nit: Capitalize https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:191: * Maybe starts {@code mRequest}, and waits for it to complete. Should be clearer here. Maybe "On first call, creates and starts URLRequest, and waits for it to complete. Does nothing on subsequent calls." https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:200: mMessageLoop.loop(); Reading over the API docs, should we really get the entire body here, or should we only block as needed? Ir block until we have the headers, then only block as the body is read? The docs don't seem to be clear here, but should presumably try and duplicate HttpURLConnection's behavior here - in particular, if it provides bytes just as they are read, rather than caching the entire file into RAM, we shouldn't be caching the entire file. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:207: if (mRequest != null) { Can mRequest be non-NULL here? https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java:30: protected URLConnection openConnection(URL u) throws IOException { nit: u -> url
Thanks for the review! Uploaded a new patch. PTAL https://codereview.chromium.org/725683002/diff/40001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet.gypi#n... components/cronet.gypi:243: 'target_name': 'cronet_tests', On 2014/11/18 15:53:51, mmenke wrote: > Why is this needed? I need this target so I canuse UploadTestServer etc in my diffing app. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:14: On 2014/11/18 15:53:52, mmenke wrote: > nit: Remove blank line, to be consistent with other cronet files (Goes for the > other Java files, too). Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:15: private final ByteBuffer mBuf; On 2014/11/18 15:53:51, mmenke wrote: > optional nit: Suggest just writing out buffer for readability. It is a common > enough abbreviation that the C++ style guide does allow for it, though. Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:18: // Makes a read-only copy of the buffer passed in. On 2014/11/18 15:53:51, mmenke wrote: > This is not actually a copy, but a read only reference to the data pointed at by > the old buffer. The java docs says it is a new instance http://developer.android.com/reference/java/nio/ByteBuffer.html#asReadOnlyBuf.... I thought a new instance means it is a new object. hmm.. but the method is abstract. maybe that depends on the implementation. What's an alternative? https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:41: private final BlockingQueue<MessageLoop.Message> mMessageQueue; On 2014/11/18 15:53:52, mmenke wrote: > nit: Don't think we need a blank line between every pair of variables. Suggest > grouping related ones. Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:43: On 2014/11/18 15:53:52, mmenke wrote: > nit: Remove extra line break. Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: On 2014/11/18 15:53:52, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:57: On 2014/11/18 15:53:52, mmenke wrote: > nit: -blank line. Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:153: On 2014/11/18 15:53:52, mmenke wrote: > nit: -blank line Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:166: // TODO(xunjieli): handle streaming. On 2014/11/18 15:53:52, mmenke wrote: > nit: Capitalize Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:173: // TODO(xunjieli): handle redirect. On 2014/11/18 15:53:52, mmenke wrote: > nit: Capitalize Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:185: // TODO(xunjieli): handle failure. On 2014/11/18 15:53:52, mmenke wrote: > nit: Capitalize Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:191: * Maybe starts {@code mRequest}, and waits for it to complete. On 2014/11/18 15:53:52, mmenke wrote: > Should be clearer here. Maybe "On first call, creates and starts URLRequest, > and waits for it to complete. Does nothing on subsequent calls." Done. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:200: mMessageLoop.loop(); On 2014/11/18 15:53:52, mmenke wrote: > Reading over the API docs, should we really get the entire body here, or should > we only block as needed? Ir block until we have the headers, then only block as > the body is read? Done. Agreed, we should only block until we receive headers, and block again if we try to read the body. >The docs don't seem to be clear here, but should presumably > try and duplicate HttpURLConnection's behavior here - in particular, if it > provides bytes just as they are read, rather than caching the entire file into > RAM, we shouldn't be caching the entire file. But we still have to cache the response in memory right? Not sure if I understand your suggestion completely. But PTAL at the new patch. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:207: if (mRequest != null) { On 2014/11/18 15:53:52, mmenke wrote: > Can mRequest be non-NULL here? Yes, if setRequestProperty is called before maybeStartRequest, then mRequest is non-NULL. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLStreamHandler.java:30: protected URLConnection openConnection(URL u) throws IOException { On 2014/11/18 15:53:52, mmenke wrote: > nit: u -> url Done.
Quick responses https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:18: // Makes a read-only copy of the buffer passed in. On 2014/11/18 18:13:35, xunjieli wrote: > On 2014/11/18 15:53:51, mmenke wrote: > > This is not actually a copy, but a read only reference to the data pointed at > by > > the old buffer. > > The java docs says it is a new instance > http://developer.android.com/reference/java/nio/ByteBuffer.html#asReadOnlyBuf.... > I thought a new instance means it is a new object. hmm.. but the method is > abstract. maybe that depends on the implementation. What's an alternative? It's a new Java object, but it shares the buffer. Read the full text at that link: ...The new buffer shares its content with this buffer, which means this buffer's change of content will be visible to the new buffer. The two buffer's position, limit and mark are independent. I'm not sure if clone() would duplicate the buffer contents or not. You can always allocate a new one and copy. https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:200: mMessageLoop.loop(); On 2014/11/18 18:13:35, xunjieli wrote: > On 2014/11/18 15:53:52, mmenke wrote: > > Reading over the API docs, should we really get the entire body here, or > should > > we only block as needed? Ir block until we have the headers, then only block > as > > the body is read? > Done. Agreed, we should only block until we receive headers, and block again if > we try to read the body. > > >The docs don't seem to be clear here, but should presumably > > try and duplicate HttpURLConnection's behavior here - in particular, if it > > provides bytes just as they are read, rather than caching the entire file into > > RAM, we shouldn't be caching the entire file. > > But we still have to cache the response in memory right? Not sure if I > understand your suggestion completely. But PTAL at the new patch. Which method(s) force us to to cache the response in memory? getContent? Or is there another one I'm missing?
On 2014/11/18 18:40:59, mmenke wrote: > Quick responses > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java > (right): > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:18: > // Makes a read-only copy of the buffer passed in. > On 2014/11/18 18:13:35, xunjieli wrote: > > On 2014/11/18 15:53:51, mmenke wrote: > > > This is not actually a copy, but a read only reference to the data pointed > at > > by > > > the old buffer. > > > > The java docs says it is a new instance > > > http://developer.android.com/reference/java/nio/ByteBuffer.html#asReadOnlyBuf.... > > I thought a new instance means it is a new object. hmm.. but the method is > > abstract. maybe that depends on the implementation. What's an alternative? > > It's a new Java object, but it shares the buffer. Read the full text at that > link: > > ...The new buffer shares its content with this buffer, which means this buffer's > change of content will be visible to the new buffer. The two buffer's position, > limit and mark are independent. > > I'm not sure if clone() would duplicate the buffer contents or not. You can > always allocate a new one and copy. Sounds good. I will allocate and make a new one. > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:200: > mMessageLoop.loop(); > On 2014/11/18 18:13:35, xunjieli wrote: > > On 2014/11/18 15:53:52, mmenke wrote: > > > Reading over the API docs, should we really get the entire body here, or > > should > > > we only block as needed? Ir block until we have the headers, then only > block > > as > > > the body is read? > > Done. Agreed, we should only block until we receive headers, and block again > if > > we try to read the body. > > > > >The docs don't seem to be clear here, but should presumably > > > try and duplicate HttpURLConnection's behavior here - in particular, if it > > > provides bytes just as they are read, rather than caching the entire file > into > > > RAM, we shouldn't be caching the entire file. > > > > But we still have to cache the response in memory right? Not sure if I > > understand your suggestion completely. But PTAL at the new patch. > > Which method(s) force us to to cache the response in memory? getContent? Or is > there another one I'm missing? getInputStream returns a an InputStream that represents the response body. We are copying the buffer in onDataReceived(), which is essentially caching the response body in memory right?
On 2014/11/18 18:44:41, xunjieli wrote: > On 2014/11/18 18:40:59, mmenke wrote: > > Quick responses > > > > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > > File > > > components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java > > (right): > > > > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > > > components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:18: > > // Makes a read-only copy of the buffer passed in. > > On 2014/11/18 18:13:35, xunjieli wrote: > > > On 2014/11/18 15:53:51, mmenke wrote: > > > > This is not actually a copy, but a read only reference to the data pointed > > at > > > by > > > > the old buffer. > > > > > > The java docs says it is a new instance > > > > > > http://developer.android.com/reference/java/nio/ByteBuffer.html#asReadOnlyBuf.... > > > I thought a new instance means it is a new object. hmm.. but the method is > > > abstract. maybe that depends on the implementation. What's an alternative? > > > > It's a new Java object, but it shares the buffer. Read the full text at that > > link: > > > > ...The new buffer shares its content with this buffer, which means this > buffer's > > change of content will be visible to the new buffer. The two buffer's > position, > > limit and mark are independent. > > > > I'm not sure if clone() would duplicate the buffer contents or not. You can > > always allocate a new one and copy. > > Sounds good. I will allocate and make a new one. > > > > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > > File > > > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > > (right): > > > > > https://codereview.chromium.org/725683002/diff/40001/components/cronet/androi... > > > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:200: > > mMessageLoop.loop(); > > On 2014/11/18 18:13:35, xunjieli wrote: > > > On 2014/11/18 15:53:52, mmenke wrote: > > > > Reading over the API docs, should we really get the entire body here, or > > > should > > > > we only block as needed? Ir block until we have the headers, then only > > block > > > as > > > > the body is read? > > > Done. Agreed, we should only block until we receive headers, and block again > > if > > > we try to read the body. > > > > > > >The docs don't seem to be clear here, but should presumably > > > > try and duplicate HttpURLConnection's behavior here - in particular, if it > > > > provides bytes just as they are read, rather than caching the entire file > > into > > > > RAM, we shouldn't be caching the entire file. > > > > > > But we still have to cache the response in memory right? Not sure if I > > > understand your suggestion completely. But PTAL at the new patch. > > > > Which method(s) force us to to cache the response in memory? getContent? Or > is > > there another one I'm missing? > > getInputStream returns a an InputStream that represents the response body. We > are copying the buffer in onDataReceived(), which is essentially caching the > response body in memory right? We could only read data from the network as it's requested by the embedder. i.e. (Sorry, using C++ style here) int Read() { if (!mBuffer.hasRemaining() && !mClosed) { RunMessageLoopUntilReadCompletesOrRequestFinishesOrFails(); } if (mBuffer.hasRemaining()) return mBuffer.get() & 0xFF; if (mClosed) return -1; } This may or may not be what the default implementation does - we should figure that out.
Thanks for the suggestion. I will try to see if I can get this working. The default one (which I assume you mean okhttp) seems to only read response body when getInputStream is invoked. There is quite a lot of layering which obscures the threading model, so I am not sure. Here are the relevant code, if you are curious. https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/ja... https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareu... On Tue, Nov 18, 2014 at 3:47 PM, <mmenke@chromium.org> wrote: > On 2014/11/18 18:44:41, xunjieli wrote: > >> On 2014/11/18 18:40:59, mmenke wrote: >> > Quick responses >> > >> > >> > > https://codereview.chromium.org/725683002/diff/40001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > ByteBufferInputStream.java > >> > File >> > >> > > components/cronet/android/java/src/org/chromium/net/urlconnection/ > ByteBufferInputStream.java > >> > (right): >> > >> > >> > > https://codereview.chromium.org/725683002/diff/40001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > ByteBufferInputStream.java#newcode18 > >> > >> > > components/cronet/android/java/src/org/chromium/net/urlconnection/ > ByteBufferInputStream.java:18: > >> > // Makes a read-only copy of the buffer passed in. >> > On 2014/11/18 18:13:35, xunjieli wrote: >> > > On 2014/11/18 15:53:51, mmenke wrote: >> > > > This is not actually a copy, but a read only reference to the data >> > pointed > >> > at >> > > by >> > > > the old buffer. >> > > >> > > The java docs says it is a new instance >> > > >> > >> > > http://developer.android.com/reference/java/nio/ByteBuffer. > html#asReadOnlyBuffer(). > >> > > I thought a new instance means it is a new object. hmm.. but the >> method is >> > > abstract. maybe that depends on the implementation. What's an >> alternative? >> > >> > It's a new Java object, but it shares the buffer. Read the full text at >> > that > >> > link: >> > >> > ...The new buffer shares its content with this buffer, which means this >> buffer's >> > change of content will be visible to the new buffer. The two buffer's >> position, >> > limit and mark are independent. >> > >> > I'm not sure if clone() would duplicate the buffer contents or not. >> You can >> > always allocate a new one and copy. >> > > Sounds good. I will allocate and make a new one. >> > >> > >> > > https://codereview.chromium.org/725683002/diff/40001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > >> > File >> > >> > > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > >> > (right): >> > >> > >> > > https://codereview.chromium.org/725683002/diff/40001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java#newcode200 > >> > >> > > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java:200: > >> > mMessageLoop.loop(); >> > On 2014/11/18 18:13:35, xunjieli wrote: >> > > On 2014/11/18 15:53:52, mmenke wrote: >> > > > Reading over the API docs, should we really get the entire body >> here, or >> > > should >> > > > we only block as needed? Ir block until we have the headers, then >> only >> > block >> > > as >> > > > the body is read? >> > > Done. Agreed, we should only block until we receive headers, and block >> > again > >> > if >> > > we try to read the body. >> > > >> > > >The docs don't seem to be clear here, but should presumably >> > > > try and duplicate HttpURLConnection's behavior here - in >> particular, if >> > it > >> > > > provides bytes just as they are read, rather than caching the entire >> > file > >> > into >> > > > RAM, we shouldn't be caching the entire file. >> > > >> > > But we still have to cache the response in memory right? Not sure if I >> > > understand your suggestion completely. But PTAL at the new patch. >> > >> > Which method(s) force us to to cache the response in memory? >> getContent? >> > Or > >> is >> > there another one I'm missing? >> > > getInputStream returns a an InputStream that represents the response >> body. We >> are copying the buffer in onDataReceived(), which is essentially caching >> the >> response body in memory right? >> > > We could only read data from the network as it's requested by the embedder. > > i.e. (Sorry, using C++ style here) > > int Read() { > if (!mBuffer.hasRemaining() && !mClosed) { > RunMessageLoopUntilReadCompletesOrRequestFinishesOrFails(); > } > > if (mBuffer.hasRemaining()) > return mBuffer.get() & 0xFF; > if (mClosed) > return -1; > } > > This may or may not be what the default implementation does - we should > figure > that out. > > https://codereview.chromium.org/725683002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/11/18 21:02:03, xunjieli wrote: > Thanks for the suggestion. I will try to see if I can get this working. > The default one (which I assume you mean okhttp) seems to only read > response body when getInputStream is invoked. There is quite a lot of > layering which obscures the threading model, so I am not sure. Here are the > relevant code, if you are curious. > https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/ja... > https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareu... I was actually thinking manual testing - make a server very slowly sends data, and see if we get it piecemeal before we've received the entire response.
>I was actually thinking manual testing - make a server very slowly sends data, >and see if we get it piecemeal before we've received the entire response. I did not use/create a slow server. But I tried with a request to http://chromium.org. I do see we get multiple onDataReceived() calls, and it seems that we are correctly piecing them together and feeding into the inputstream. Here's the new patch. PTAL. Thanks! https://codereview.chromium.org/725683002/diff/100001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/100001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:164: mResponseByteBuffer = ByteBuffer.allocateDirect(byteBuffer.capacity()); I thought about reusing old buffer, but the size of byte buffers received in onDataReceived() call varies quite a bit. Since we cannot grow a byte buffer, I decided to allocate new byte buffer. (I will wrap this line in the next patch.)
Sorry for slowness. Looks pretty good. Haven't yet dug into tests. https://codereview.chromium.org/725683002/diff/120001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet.gypi#... components/cronet.gypi:243: 'target_name': 'cronet_tests', I don't think this belongs in this CL - maybe add your diff tool in a future CL, and add this in that CL, too? Could base tests on it as well. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:13: class ByteBufferInputStream extends InputStream { Suggest a more specific name, since this isn't really a general InputStream that uses a ByteBuffer. Maybe just CronetInputStream or Cronet[Http]URLConnectionInputStream, if we really want to be complete. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:27: mMessageLoop.loop(); Rather than using the message loop directly, I think it makes more sense to call into a private method in CronetHttpURLConnection to request more data. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:32: return -1; If there was a failure rather than a successful completion, should we be throwing an exception? I guess you have TODOs about this elsewhere. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:36: private ByteBufferInputStream mResponseBody; Name here is confusing. Maybe just mInputStream? https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:68: public void connect() throws IOException { Question: With the default implementation, I assume connect() can't be used more than once on the same object, to re-request a resource? https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:111: maybeStartRequest(); optional nit: Think this makes more sense in the body of the if. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:149: nit: Remove blank line. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:164: mResponseByteBuffer = I don't think we currently need a member variable for this. Alternatively, we could have the ByteBufferInputStream call into |this| both to get the byte buffer, and to access the current state of the connection, rather than using a push-based model. May make the code a little simpler. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:165: ByteBuffer.allocateDirect(byteBuffer.capacity()); Should we be using allocate here instead? https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:188: setResponseDataCompleted(); I think it's weird that onSucceeded/onFailed have the outer class call into the buffer and exit the message loop, while onDataReceived does those itself. I'm fine with either the inner our outer class do all the work, just think we should be consistent. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:202: connected = true; Does it make sense to do this even in the case we fail to establish a connection? The docs aren't clear on this topic. Know you aren't currently handling errors, anyways. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:210: private void maybeCreateRequest() { Can we just create this in the constructor, rather than doing lazy initialization? Don't think we get enough out of the lazy initialization to be worth the complexity. This adds the risk that any feature that requires the request exist before we use it will result in a regression.
Thanks for the review! PTAL https://codereview.chromium.org/725683002/diff/120001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet.gypi#... components/cronet.gypi:243: 'target_name': 'cronet_tests', On 2014/11/20 18:52:36, mmenke wrote: > I don't think this belongs in this CL - maybe add your diff tool in a future CL, > and add this in that CL, too? Could base tests on it as well. Done. Removed from this CL. Will probably still have my diff tool not in the code repo, since it is only for debugging/manual testing purposes. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:13: class ByteBufferInputStream extends InputStream { On 2014/11/20 18:52:36, mmenke wrote: > Suggest a more specific name, since this isn't really a general InputStream that > uses a ByteBuffer. Maybe just CronetInputStream or > Cronet[Http]URLConnectionInputStream, if we really want to be complete. Done. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:27: mMessageLoop.loop(); On 2014/11/20 18:52:36, mmenke wrote: > Rather than using the message loop directly, I think it makes more sense to call > into a private method in CronetHttpURLConnection to request more data. Done. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/ByteBufferInputStream.java:32: return -1; On 2014/11/20 18:52:36, mmenke wrote: > If there was a failure rather than a successful completion, should we be > throwing an exception? I guess you have TODOs about this elsewhere. Acknowledged. Right, I think I will handle failure in a follow-up CL. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:36: private ByteBufferInputStream mResponseBody; On 2014/11/20 18:52:37, mmenke wrote: > Name here is confusing. Maybe just mInputStream? Done. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:68: public void connect() throws IOException { On 2014/11/20 18:52:37, mmenke wrote: > Question: With the default implementation, I assume connect() can't be used > more than once on the same object, to re-request a resource? Right, the documentation says if connect() is called a second time, the call is ignored (http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/...). In maybeStartRequest(), I checked whether connected is true. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:111: maybeStartRequest(); On 2014/11/20 18:52:37, mmenke wrote: > optional nit: Think this makes more sense in the body of the if. Done. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:149: On 2014/11/20 18:52:37, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:164: mResponseByteBuffer = On 2014/11/20 18:52:37, mmenke wrote: > I don't think we currently need a member variable for this. Alternatively, we > could have the ByteBufferInputStream call into |this| both to get the byte > buffer, and to access the current state of the connection, rather than using a > push-based model. May make the code a little simpler. Done. Changed to pull-based model, so we have one fewer method. Thanks for the suggestion https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:165: ByteBuffer.allocateDirect(byteBuffer.capacity()); On 2014/11/20 18:52:37, mmenke wrote: > Should we be using allocate here instead? Done. Not exactly sure which one to use. The original byte buffer is a direct bytebuffer, so I did allocateDirect(). But I guess this bytebuffer copy is on java side the whole time, so allocate() should be equivalent. https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:188: setResponseDataCompleted(); On 2014/11/20 18:52:36, mmenke wrote: > I think it's weird that onSucceeded/onFailed have the outer class call into the > buffer and exit the message loop, while onDataReceived does those itself. I'm > fine with either the inner our outer class do all the work, just think we should > be consistent. Done. Havent noticed that. thanks! https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:202: connected = true; On 2014/11/20 18:52:37, mmenke wrote: > Does it make sense to do this even in the case we fail to establish a > connection? The docs aren't clear on this topic. Know you aren't currently > handling errors, anyways. Done. I think it makes sense to set connected to true no matter whether the request succeeds or fails. I moved the assignment to before mRequest.start(). https://codereview.chromium.org/725683002/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:210: private void maybeCreateRequest() { On 2014/11/20 18:52:37, mmenke wrote: > Can we just create this in the constructor, rather than doing lazy > initialization? Don't think we get enough out of the lazy initialization to be > worth the complexity. This adds the risk that any feature that requires the > request exist before we use it will result in a regression. Done. Good idea! thanks.
https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:34: private final UrlRequest mRequest; optional: Maybe another line break here, to emphasize the value above this is final, unlike the information about the response. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:84: mInputStream.close(); mInputStream = null, so we handle multiple calls? Also, what if this is called before connect(), should we not do anything in connect, or throw an exception or something? What does the default implementation do? https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:95: public String getResponseMessage() { We don't have any tests exercise this. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:96: maybeStartRequest(); It's a pain, but we should probably test all these implicit start calls. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:154: * read. Worth noting that this may only be called after the request has started. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:156: ByteBuffer requestData() { maybe requestMoreData? ("Request data" could be a noun, with request as an adjective, or a verb phrase, with request as a verb. The "more" removes that ambiguity). Or maybe just getMoreData, since we aren't actually sending the request here. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:26: * Example test that just starts the cronet sample. nit: capitalize Cronet? https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:28: public class CronetHttpURLConnectionTest extends CronetTestBase { Do instrumentation tests support parameterizing test cases? i.e. run every test with a variable or enum having one value and another? I'd really like to be able to make a test suite that runs all tests with our wrapper and the default implementation, to validate they behave in the same way. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:56: assertEquals(200, urlConnection.getResponseCode()); Should have a non-2xx response test, too, with a body. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:59: } catch (Exception e) { Why are these needed? Mother other tests just let the test fixture get the exception. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:73: urlConnection.addRequestProperty("header2", "bar"); optional: Using foo-header and bar-header instead of header1 and header2 may make this test a little easier to read. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:157: } Other test suggestion: connect -> disconnect -> try reading. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:157: } Other test suggestion: It's an issue with out other tests, too, but none of these tests results in multiple body reads. Fine to think about this in another CL, since this may require some test changes. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:169: out.append(line + "\n"); Why append a line break here?
Matt, I addressed some of the issues. Two major ones remaining: #1. Connect -> Disconnect -> then read data. #2. Parameterized test cases so we can test the default implementation at the same time. Please take a look at inline comments. Thanks! https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:34: private final UrlRequest mRequest; On 2014/11/21 16:53:07, mmenke wrote: > optional: Maybe another line break here, to emphasize the value above this is > final, unlike the information about the response. Done. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:84: mInputStream.close(); On 2014/11/21 16:53:07, mmenke wrote: > mInputStream = null, so we handle multiple calls? Also, what if this is called > before connect(), should we not do anything in connect, or throw an exception or > something? What does the default implementation do? OkHttp does nothing if it is called multiple times or it is called before connect(). See http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa.... I am not sure if I should do mRequest.cancel() here, it seems that I should not do that. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:95: public String getResponseMessage() { On 2014/11/21 16:53:07, mmenke wrote: > We don't have any tests exercise this. Done. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:96: maybeStartRequest(); On 2014/11/21 16:53:07, mmenke wrote: > It's a pain, but we should probably test all these implicit start calls. Acknowledged. Will keep this in mind and handle it after fixing other test issues. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:154: * read. On 2014/11/21 16:53:07, mmenke wrote: > Worth noting that this may only be called after the request has started. Done. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:156: ByteBuffer requestData() { On 2014/11/21 16:53:06, mmenke wrote: > maybe requestMoreData? ("Request data" could be a noun, with request as an > adjective, or a verb phrase, with request as a verb. The "more" removes that > ambiguity). > > Or maybe just getMoreData, since we aren't actually sending the request here. Done. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:26: * Example test that just starts the cronet sample. On 2014/11/21 16:53:07, mmenke wrote: > nit: capitalize Cronet? Done. Just realize that this is a stale comment. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:28: public class CronetHttpURLConnectionTest extends CronetTestBase { On 2014/11/21 16:53:07, mmenke wrote: > Do instrumentation tests support parameterizing test cases? i.e. run every test > with a variable or enum having one value and another? I'd really like to be > able to make a test suite that runs all tests with our wrapper and the default > implementation, to validate they behave in the same way. I did some search, I think the standard way is to implement our own instrumentation test runner. See http://stackoverflow.com/questions/14820175/how-to-pass-an-argument-to-a-andr.... If we choose to do so, we will enable a command line flag on our buildbot to run the same set of urlconnection tests for the default implementation right? https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:56: assertEquals(200, urlConnection.getResponseCode()); On 2014/11/21 16:53:07, mmenke wrote: > Should have a non-2xx response test, too, with a body. Done. Did a 404 response. But I used MockUrlRequestJobFactory, which won't work for the default implementation. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:59: } catch (Exception e) { On 2014/11/21 16:53:07, mmenke wrote: > Why are these needed? Mother other tests just let the test fixture get the > exception. Done. Yes, you are right. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:73: urlConnection.addRequestProperty("header2", "bar"); On 2014/11/21 16:53:07, mmenke wrote: > optional: Using foo-header and bar-header instead of header1 and header2 may > make this test a little easier to read. Done. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:157: } On 2014/11/21 16:53:07, mmenke wrote: > Other test suggestion: It's an issue with out other tests, too, but none of > these tests results in multiple body reads. Fine to think about this in another > CL, since this may require some test changes. What do you mean by multiple body reads? read the inputstream after it has been read once? https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:157: } On 2014/11/21 16:53:07, mmenke wrote: > Other test suggestion: connect -> disconnect -> try reading. Done. Currently the default implementation throws an SocketException (java.net.SocketException: sendto failed: EBADF Bad file number), and ours proceed as if everything's fine. What is the best way to deal with this? Should we add a boolean flag in CronetHttpURLConnection that basically says whether the connection has been disconnected, if it is, throw an exception when user try to read response code/message/body? https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:169: out.append(line + "\n"); On 2014/11/21 16:53:07, mmenke wrote: > Why append a line break here? so reader.readLine() returns me a line without the original line breaks. I add one back so i can parse headers. Otherwise, headers will be like "header1:value1header2:value2".
A couple responses. I'm fine with landing this without it, but before we start fleshing out the implementation some more, I do think we want a test fixture that can compare the default and out implementations of both methods. We'll want to reuse it for our wrapper around HttpURLConnection as well. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:84: mInputStream.close(); On 2014/11/21 20:30:55, xunjieli wrote: > On 2014/11/21 16:53:07, mmenke wrote: > > mInputStream = null, so we handle multiple calls? Also, what if this is > called > > before connect(), should we not do anything in connect, or throw an exception > or > > something? What does the default implementation do? > > OkHttp does nothing if it is called multiple times or it is called before > connect(). See > http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa.... > I am not sure if I should do mRequest.cancel() here, it seems that I should not > do that. Hrm...ok, so looks like we're copying their behavior, weird though it is. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:157: } On 2014/11/21 20:30:56, xunjieli wrote: > On 2014/11/21 16:53:07, mmenke wrote: > > Other test suggestion: It's an issue with out other tests, too, but none of > > these tests results in multiple body reads. Fine to think about this in > another > > CL, since this may require some test changes. > > What do you mean by multiple body reads? read the inputstream after it has been > read once? I was thinking multiple reads at the CronetURLRequest, actually. Multiple reads from the InputStream, and making sure we exercise both read methods, is also something we should be doing (Not clear which BufferedReader uses, or if it reads one byte at a time behind the scenes or not - best to test them manually). Reading with the single byte method, trying to read more bytes at once than are currently available, reading fewer bytes than are currently available (And then reading more, when we're at an offset), reading the exact number of bytes currently available should all be tested. ByteBuffer makes some of these variations less exciting than they'd be if we didn't have it, but should still test them as if we were doing all that logic ourselves. https://codereview.chromium.org/725683002/diff/140001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:169: out.append(line + "\n"); On 2014/11/21 20:30:56, xunjieli wrote: > On 2014/11/21 16:53:07, mmenke wrote: > > Why append a line break here? > > so reader.readLine() returns me a line without the original line breaks. I add > one back so i can parse headers. Otherwise, headers will be like > "header1:value1header2:value2". Ahh...Suggest just reading the data directly, so we don't have to muck with that. Checking for "GET\n", for instance, seems weird, and may have the potential to overlook certain bugs.
Thanks for the suggestions! I will work on the tests now. On Fri, Nov 21, 2014 at 4:26 PM, <mmenke@chromium.org> wrote: > A couple responses. I'm fine with landing this without it, but before we > start > fleshing out the implementation some more, I do think we want a test > fixture > that can compare the default and out implementations of both methods. > We'll > want to reuse it for our wrapper around HttpURLConnection as well. > > > https://codereview.chromium.org/725683002/diff/140001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > File > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/725683002/diff/140001/ > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java#newcode84 > components/cronet/android/java/src/org/chromium/net/urlconnection/ > CronetHttpURLConnection.java:84: > mInputStream.close(); > On 2014/11/21 20:30:55, xunjieli wrote: > >> On 2014/11/21 16:53:07, mmenke wrote: >> > mInputStream = null, so we handle multiple calls? Also, what if >> > this is > >> called >> > before connect(), should we not do anything in connect, or throw an >> > exception > >> or >> > something? What does the default implementation do? >> > > OkHttp does nothing if it is called multiple times or it is called >> > before > >> connect(). See >> > > http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/ > main/java/com/squareup/okhttp/internal/http/HttpURLConnectionImpl.java#93. > >> I am not sure if I should do mRequest.cancel() here, it seems that I >> > should not > >> do that. >> > > Hrm...ok, so looks like we're copying their behavior, weird though it > is. > > https://codereview.chromium.org/725683002/diff/140001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java > (right): > > https://codereview.chromium.org/725683002/diff/140001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java#newcode157 > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java:157: > } > On 2014/11/21 20:30:56, xunjieli wrote: > >> On 2014/11/21 16:53:07, mmenke wrote: >> > Other test suggestion: It's an issue with out other tests, too, but >> > none of > >> > these tests results in multiple body reads. Fine to think about >> > this in > >> another >> > CL, since this may require some test changes. >> > > What do you mean by multiple body reads? read the inputstream after it >> > has been > >> read once? >> > > I was thinking multiple reads at the CronetURLRequest, actually. > > Multiple reads from the InputStream, and making sure we exercise both > read methods, is also something we should be doing (Not clear which > BufferedReader uses, or if it reads one byte at a time behind the scenes > or not - best to test them manually). > > Reading with the single byte method, trying to read more bytes at once > than are currently available, reading fewer bytes than are currently > available (And then reading more, when we're at an offset), reading the > exact number of bytes currently available should all be tested. > > ByteBuffer makes some of these variations less exciting than they'd be > if we didn't have it, but should still test them as if we were doing all > that logic ourselves. > > https://codereview.chromium.org/725683002/diff/140001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java#newcode169 > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > urlconnection/CronetHttpURLConnectionTest.java:169: > out.append(line + "\n"); > On 2014/11/21 20:30:56, xunjieli wrote: > >> On 2014/11/21 16:53:07, mmenke wrote: >> > Why append a line break here? >> > > so reader.readLine() returns me a line without the original line >> > breaks. I add > >> one back so i can parse headers. Otherwise, headers will be like >> "header1:value1header2:value2". >> > > Ahh...Suggest just reading the data directly, so we don't have to muck > with that. Checking for "GET\n", for instance, seems weird, and may > have the potential to overlook certain bugs. > > https://codereview.chromium.org/725683002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #10 (id:180001) has been deleted
Matt, I have added test support to compare default implementation with Cronet's wrapper. I also tested reading from the InputStream. PTAL. Thanks!
On 2014/11/24 18:52:49, xunjieli wrote: > Matt, I have added test support to compare default implementation with Cronet's > wrapper. I also tested reading from the InputStream. PTAL. Thanks! I'll get to this later today. Want to take a very careful look, hoping this will be my last full pass.
Sounds good! thanks! On Tue, Nov 25, 2014 at 10:25 AM, <mmenke@chromium.org> wrote: > On 2014/11/24 18:52:49, xunjieli wrote: > >> Matt, I have added test support to compare default implementation with >> > Cronet's > >> wrapper. I also tested reading from the InputStream. PTAL. Thanks! >> > > I'll get to this later today. Want to take a very careful look, hoping > this > will be my last full pass. > > https://codereview.chromium.org/725683002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry, started another pass, but won't have comments until tomorrow.
Couple of quick comments. Also, is it really a good idea to catch exceptions and print their stack traces? Wouldn't app be interested in catching them on its own? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:50: * Wrapper executor class which posts tasks to current looper. posts tasks to message queue? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: private class HandlerThreadExecutor implements Executor { Maybe rename to MessageLoopExecutor? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:86: e.printStackTrace(); is it unexpected to throw here? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:173: public void onDataReceived(UrlRequest request, ResponseInfo info, BUG? This will be called many times, we need to accumulate the content of byteBuffer. Are we expecting that it will not get called again until we run message loop again? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:31: UrlRequestContextConfig config = new UrlRequestContextConfig(); I suppose we should allow embedder to somehow supply config. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:38: * Returns null for protocols other than http and https. nit: need better wording (e.g. what does it do for http).
https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:32: private final BlockingQueue<MessageLoop.Message> mMessageQueue; Rather than having the CronetHttpURLConnection own the queue, I think it's cleaner to have the queue be an internal details of the MessageLoop implementation. Add a postMessage or postTask method to the MessageLoop, which takes a Runnable...And throws an exception if it's NULL (My general feeling is we should have only one way to do something, unless we need two, as it makes it easier to navigate the code, and we already have MessageLoop.quit()). https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:24: public void loop() { Maybe throw an exception if already running on another thread? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:35: e.printStackTrace(); Should we really just be silently eating these? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:44: public void quit() { postQuitMessage? That makes it clearer it doesn't quit instantly. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:55: static class Message { per other comment, suggest making this private. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:55: static class Message { Alternatively... Charles was concerned about making a bunch of short-lived objects, which resulted in us re-using Runnables for every read. Could just get rid of this class, and make the queue have runnables. How to handle quit, then? Have a mLoopRunning bool or somesuch, and make a reuseable runnable to set it to true. Could also check it's false in loop, to avoid re-entrancy. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:30: * "OnlyRunCronetHttpURLConnection" only run Cronet's implementation. Suggest adding something along the lines of "See CronetTestBase for logic to do this."
One major issue remaining is how we should handle exceptions, particularly those in MessageLoop.java. Thoughts? PTAL. thanks! https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:32: private final BlockingQueue<MessageLoop.Message> mMessageQueue; On 2014/11/26 14:59:46, mmenke wrote: > Rather than having the CronetHttpURLConnection own the queue, I think it's > cleaner to have the queue be an internal details of the MessageLoop > implementation. > > Add a postMessage or postTask method to the MessageLoop, which takes a > Runnable...And throws an exception if it's NULL (My general feeling is we should > have only one way to do something, unless we need two, as it makes it easier to > navigate the code, and we already have MessageLoop.quit()). Done. Thanks for the suggestion! https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:50: * Wrapper executor class which posts tasks to current looper. On 2014/11/25 23:04:22, mef wrote: > posts tasks to message queue? Done. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: private class HandlerThreadExecutor implements Executor { On 2014/11/25 23:04:22, mef wrote: > Maybe rename to MessageLoopExecutor? Done. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:86: e.printStackTrace(); On 2014/11/25 23:04:22, mef wrote: > is it unexpected to throw here? InputStream.close() throws an exception, it has to be caught, since disconnect() does not throw Exception. What's an alternative? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:173: public void onDataReceived(UrlRequest request, ResponseInfo info, On 2014/11/25 23:04:22, mef wrote: > BUG? This will be called many times, we need to accumulate the content of > byteBuffer. Are we expecting that it will not get called again until we run > message loop again? Yes, it will not get called again util we run the message loop, since future onDataReceived() callbacks are executed on the same message loop. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:31: UrlRequestContextConfig config = new UrlRequestContextConfig(); On 2014/11/25 23:04:23, mef wrote: > I suppose we should allow embedder to somehow supply config. Acknowledged. Yes, we need to have a way to do that. I added a TODO https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetURLStreamHandlerFactory.java:38: * Returns null for protocols other than http and https. On 2014/11/25 23:04:22, mef wrote: > nit: need better wording (e.g. what does it do for http). Done. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:24: public void loop() { On 2014/11/26 14:59:46, mmenke wrote: > Maybe throw an exception if already running on another thread? Done. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:44: public void quit() { On 2014/11/26 14:59:46, mmenke wrote: > postQuitMessage? That makes it clearer it doesn't quit instantly. Done. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:55: static class Message { On 2014/11/26 14:59:46, mmenke wrote: > Alternatively... Charles was concerned about making a bunch of short-lived > objects, which resulted in us re-using Runnables for every read. Could just get > rid of this class, and make the queue have runnables. How to handle quit, then? > Have a mLoopRunning bool or somesuch, and make a reuseable runnable to set it > to true. > > Could also check it's false in loop, to avoid re-entrancy. Done. That's neat! thanks! https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:30: * "OnlyRunCronetHttpURLConnection" only run Cronet's implementation. On 2014/11/26 14:59:46, mmenke wrote: > Suggest adding something along the lines of "See CronetTestBase for logic to do > this." Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:46: throw new RuntimeException(e); I am not sure what I should do to handle InterruptedException. It is a checked exception, so I can't rethrow it as a runtime exception. Method signatures of HttpURLConnection do not have throw causes. Thoughts?
https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:86: e.printStackTrace(); On 2014/11/26 16:11:55, xunjieli wrote: > On 2014/11/25 23:04:22, mef wrote: > > is it unexpected to throw here? > > InputStream.close() throws an exception, it has to be caught, since disconnect() > does not throw Exception. What's an alternative? Never mind, I didn't realize that mInputStream is Cronet-specific and created in getInputStream. Should we also set it to null here? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:127: setRequestProperty(key, value); maybe add TODO describing expected behavior that we'll have to implement? https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:173: public void onDataReceived(UrlRequest request, ResponseInfo info, On 2014/11/26 16:11:55, xunjieli wrote: > On 2014/11/25 23:04:22, mef wrote: > > BUG? This will be called many times, we need to accumulate the content of > > byteBuffer. Are we expecting that it will not get called again until we run > > message loop again? > > Yes, it will not get called again util we run the message loop, since future > onDataReceived() callbacks are executed on the same message loop. sg, thanks for explanation. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:46: throw new RuntimeException(e); On 2014/11/26 16:11:56, xunjieli wrote: > I am not sure what I should do to handle InterruptedException. It is a checked > exception, so I can't rethrow it as a runtime exception. Method signatures of > HttpURLConnection do not have throw causes. Thoughts? It sounds like you can get that exception when you are waiting for the task, so maybe it should quit the loop?
> It sounds like you can get that exception when you are waiting for the task, so > maybe it should quit the loop? Hmm.. but quiting the loop requires enqueuing the quit task. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:86: e.printStackTrace(); On 2014/11/26 16:36:04, mef wrote: > On 2014/11/26 16:11:55, xunjieli wrote: > > On 2014/11/25 23:04:22, mef wrote: > > > is it unexpected to throw here? > > > > InputStream.close() throws an exception, it has to be caught, since > disconnect() > > does not throw Exception. What's an alternative? > > Never mind, I didn't realize that mInputStream is Cronet-specific and created in > getInputStream. > Should we also set it to null here? Acknowledged. Sounds good. I am planning to handle disconnect() more gracefully in a follow-up CL. Right now, if we do connect() -> disconnect() -> read response, it will still work. If we set mInputStream == null, then try to getInputStream(), a new instance of inputstream will be made, which is kinda weird. Left a comment in the tests documenting that disconnect() needs to be worked on. https://codereview.chromium.org/725683002/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:127: setRequestProperty(key, value); On 2014/11/26 16:36:04, mef wrote: > maybe add TODO describing expected behavior that we'll have to implement? will do. thanks!
Still need to look at tests again. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:73: public void disconnect() { We should also cancel the URLRequest if started. This makes sure its resources are freed. Otherwise, disconnecting after a request is started results in a leak. If not started, should we make sure we don't start it in the future? Should match whatever the other implementation does. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:73: public void disconnect() { Should we set connected to false here? I assume we should leave it as true after a request successfully completes. What if a request fails? Also note that setting connected to false here would break maybeStartRequest, so would need changes there, too. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:98: return mResponseInfo.getHttpStatusCode(); Note that these currently throw null pointer exceptions if a request fails without getting headers. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:150: return mResponseByteBuffer; Suggest setting mResponseByteBuffer to NULL before returning it. Having joint ownership of the old buffer just seems a recipe for problems. When we end up returning the last used mResponseByteBuffer (On failure or complete), the InputStream should already have read all of it, but it just seems like asking for trouble. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:170: mResponseByteBuffer.rewind(); Suggest using mResponseByteBuffer.flip() instead - think it's more common when switching from reading to writing a buffer. Also, if we switch to reusing byte buffers, it's the one we'll probably need. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:178: mResponseInfo = info; If we follow a couple redirects, and then get an error, getResponseCode and the like will return the value for the last successfully received redirect. Is this what we should be doing? If not, should just remove this line. And should add a TODO about testing this (requires test infrastructure changes, I suspect) https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:182: public void onSucceeded(UrlRequest request, ExtendedResponseInfo info) { Set mResponseInfo? https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:46: throw new RuntimeException(e); On 2014/11/26 16:36:04, mef wrote: > On 2014/11/26 16:11:56, xunjieli wrote: > > I am not sure what I should do to handle InterruptedException. It is a checked > > exception, so I can't rethrow it as a runtime exception. Method signatures of > > HttpURLConnection do not have throw causes. Thoughts? > > It sounds like you can get that exception when you are waiting for the task, so > maybe it should quit the loop? Yes, it should quit the loop (Which re-throwing the error implicitly does, actually). The question is how we should surface it to the embedder: As an exception (And if so, should it be recoverable) or as a failed error. If we treat it as a recoverable exception, the mLoopRunning logic may present a problem - it's now true, so we'd have to set it to false...but we could theoretically have just posted a quit task when we get the exception, which would cause yet more problems... I suggest just treating it as an error and failure the request, for now. One way to do that: Don't catch the error here, just make loop throw InterruptedException. Catch it in the calling class, and treat it as a failure. You'll have to wrap loop calls in another method, but don't think that's a big deal. Open to other ideas.
Thanks for the review! Just realized that connect() and get*() calls all have throw clauses, so we can pass the exception to callers. Please take a look at the new patch. Thanks! https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:73: public void disconnect() { On 2014/11/26 17:09:12, mmenke wrote: > Should we set connected to false here? I assume we should leave it as true > after a request successfully completes. What if a request fails? > > Also note that setting connected to false here would break maybeStartRequest, so > would need changes there, too. We should not set connected to false, since it indicates whether a connection has been ever attempted. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:73: public void disconnect() { On 2014/11/26 17:09:12, mmenke wrote: > We should also cancel the URLRequest if started. This makes sure its resources > are freed. Otherwise, disconnecting after a request is started results in a > leak. > > If not started, should we make sure we don't start it in the future? Should > match whatever the other implementation does. Done. If not started (connected), the disconnect() should have no effect, according to the documentation of okHttp. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:98: return mResponseInfo.getHttpStatusCode(); On 2014/11/26 17:09:12, mmenke wrote: > Note that these currently throw null pointer exceptions if a request fails > without getting headers. Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:150: return mResponseByteBuffer; On 2014/11/26 17:09:12, mmenke wrote: > Suggest setting mResponseByteBuffer to NULL before returning it. Having joint > ownership of the old buffer just seems a recipe for problems. When we end up > returning the last used mResponseByteBuffer (On failure or complete), the > InputStream should already have read all of it, but it just seems like asking > for trouble. Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:170: mResponseByteBuffer.rewind(); On 2014/11/26 17:09:12, mmenke wrote: > Suggest using mResponseByteBuffer.flip() instead - think it's more common when > switching from reading to writing a buffer. Also, if we switch to reusing byte > buffers, it's the one we'll probably need. Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:178: mResponseInfo = info; On 2014/11/26 17:09:12, mmenke wrote: > If we follow a couple redirects, and then get an error, getResponseCode and the > like will return the value for the last successfully received redirect. Is this > what we should be doing? If not, should just remove this line. > > And should add a TODO about testing this (requires test infrastructure changes, > I suspect) Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:182: public void onSucceeded(UrlRequest request, ExtendedResponseInfo info) { On 2014/11/26 17:09:12, mmenke wrote: > Set mResponseInfo? Done. https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/220001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:46: throw new RuntimeException(e); On 2014/11/26 17:09:12, mmenke wrote: > On 2014/11/26 16:36:04, mef wrote: > > On 2014/11/26 16:11:56, xunjieli wrote: > > > I am not sure what I should do to handle InterruptedException. It is a > checked > > > exception, so I can't rethrow it as a runtime exception. Method signatures > of > > > HttpURLConnection do not have throw causes. Thoughts? > > > > It sounds like you can get that exception when you are waiting for the task, > so > > maybe it should quit the loop? > > Yes, it should quit the loop (Which re-throwing the error implicitly does, > actually). The question is how we should surface it to the embedder: As an > exception (And if so, should it be recoverable) or as a failed error. > > If we treat it as a recoverable exception, the mLoopRunning logic may present a > problem - it's now true, so we'd have to set it to false...but we could > theoretically have just posted a quit task when we get the exception, which > would cause yet more problems... > > I suggest just treating it as an error and failure the request, for now. > > One way to do that: Don't catch the error here, just make loop throw > InterruptedException. Catch it in the calling class, and treat it as a failure. > You'll have to wrap loop calls in another method, but don't think that's a big > deal. > > Open to other ideas. Done.
https://codereview.chromium.org/725683002/diff/240001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/240001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:109: return mResponseInfo.getHttpStatusText(); I should probably add a null check here, and only return the status text/code if mResponseInfo is not null. Will do it in next patch. Since NPE will result in a crash.
Looking at my CL this morning, I think my exception handling in CronetHttpURLConnection doesn't look quite right, and I don't really know how I can make it less confusing. Any thoughts? https://codereview.chromium.org/725683002/diff/280001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/280001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:35: private InterruptedException mInterruptedException; My exception handling still looks weird. Not exactly sure whether this is the way to surface to callers. Any ideas?
Patchset #15 (id:300001) has been deleted
Patchset #14 (id:280001) has been deleted
https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:58: mMessageLoop.postTask(command); I feel that referencing mMessageLoop from the parent class is a little confusing. Would it make sense to make MessageLoopExecutor static and pass MessageLoop to the executor in constructor? https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:241: private void startMessageLoop() throws IOException { Doesn't it block until loop is done? Would it make sense to name it 'runMessageLoop()'? https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:243: mMessageLoop.loop(); Should we rethrow all exceptions thrown from the loop as IOException? Also, what happens if let's say getResponseCode throws an exception, and embedder calls getResponseMessage after that?
Thanks for the review! I have uploaded a new patch to address the comments. PTAL. thanks! https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:58: mMessageLoop.postTask(command); On 2014/12/01 18:28:54, mef wrote: > I feel that referencing mMessageLoop from the parent class is a little > confusing. Would it make sense to make MessageLoopExecutor static and pass > MessageLoop to the executor in constructor? Done. https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:241: private void startMessageLoop() throws IOException { On 2014/12/01 18:28:54, mef wrote: > Doesn't it block until loop is done? > Would it make sense to name it 'runMessageLoop()'? Done. https://codereview.chromium.org/725683002/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:243: mMessageLoop.loop(); On 2014/12/01 18:28:54, mef wrote: > Should we rethrow all exceptions thrown from the loop as IOException? > Also, what happens if let's say getResponseCode throws an exception, and > embedder calls getResponseMessage after that? Done. I guess the caller will need to handle IOException by either rethrowing or using try-catch block. If both getResponseCode() and getResponseMessage() are in one try-catch block, then the second one won't be invoked since there will be an exception. If these two methods are in two different try-catch, then getResponseMessage() will be run.
Not sure how to improve your exception logic. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: private final MessageLoop mLoop; nit: Blank line between member variables and methods. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:89: public void disconnect() { Disconnect isn't supposed to be threadsafe, right? That is, if you have one thread blocking waiting for input, the other can't call disconnect and expect the other thread to wake up. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:48: throw new IOException(e); This doesn't support resumption after an InterruptedException is thrown (If there's a quit message in the queue, for instance, the next attempt to run the loop quits instantly). Do other implementations allow more data to be read after an interrupted exception is thrown? https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:80: mockFactory.tearDown(); Should be able to use the test server with ServeFilesFromDirectory to run this test with both implementations. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:95: assertEquals("GET", getResponseAsString(urlConnection)); What happens if we try to read from both implementations? I assume we should get exceptions with both? Should test that. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:101: // we need to change the behavior. nit: Don't use our or we in comments. Alternative: "Currently the wrapper does not throw an exception. Need to change that behavior." https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:144: // TODO(xunjieli): change the annotation once crbug.com/432719 is fixed. nit: Comments should be capitalized. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:185: public void testAddAndSetRequestProperty() throws Exception { Should we do a test with two set calls for the same property? Could see add/set working, but not set/set. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:237: int bytesRead = in.read(actualOutput, 0, actualOutput.length); Should check that the next read (Using the same read operation) returns -1. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:281: bytesRead = in.read(actualOutput, actualOutput.length - 10, 10); Should use different destination buffers for the two reads, to make sure we don't write beyond the end on the first read.
Thanks for the review! I have uploaded a new patch to address the comments. PTAL https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:52: private final MessageLoop mLoop; On 2014/12/02 19:01:49, mmenke wrote: > nit: Blank line between member variables and methods. Done. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:89: public void disconnect() { On 2014/12/02 19:01:49, mmenke wrote: > Disconnect isn't supposed to be threadsafe, right? That is, if you have one > thread blocking waiting for input, the other can't call disconnect and expect > the other thread to wake up. It's not mentioned anywhere in the documentations, so I'd assume it does not have thread-safe guarantees. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:48: throw new IOException(e); On 2014/12/02 19:01:49, mmenke wrote: > This doesn't support resumption after an InterruptedException is thrown (If > there's a quit message in the queue, for instance, the next attempt to run the > loop quits instantly). Do other implementations allow more data to be read > after an interrupted exception is thrown? OkHttp's implementation retries until the request succeeds (http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa...). However, if it encounters an non-recoverable IOException, okHttp just rethrows it to the caller. The reason that we have this InterruptedException here is that we are using a LinkedBlockingQueue which blocks when there is no task to run. OkHttp's does not have a blocking queue, so it does not need to handle InterruptedException explicitly. I think it might be safe for now to just treat InterruptedException as an unrecoverable IOException, since it might be the caller who explicitly interrupts the thread. From the documentation (https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html), it does not mention anything about resuming. Also the Thread#resume() method is deprecated (see http://developer.android.com/reference/java/lang/Thread.html#resume()), so we probably don't need to worry about resuming. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:80: mockFactory.tearDown(); On 2014/12/02 19:01:49, mmenke wrote: > Should be able to use the test server with ServeFilesFromDirectory to run this > test with both implementations. Acknowledged. Since the support is not ready, I have added a TODO, and will do it in a follow-up CL. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:95: assertEquals("GET", getResponseAsString(urlConnection)); On 2014/12/02 19:01:49, mmenke wrote: > What happens if we try to read from both implementations? I assume we should > get exceptions with both? Should test that. I don't understand. I thought we are reading from the connection in the getResponseAsString method. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:101: // we need to change the behavior. On 2014/12/02 19:01:49, mmenke wrote: > nit: Don't use our or we in comments. Alternative: > > "Currently the wrapper does not throw an exception. Need to change that > behavior." Done. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:144: // TODO(xunjieli): change the annotation once crbug.com/432719 is fixed. On 2014/12/02 19:01:49, mmenke wrote: > nit: Comments should be capitalized. Done. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:185: public void testAddAndSetRequestProperty() throws Exception { On 2014/12/02 19:01:49, mmenke wrote: > Should we do a test with two set calls for the same property? Could see add/set > working, but not set/set. Done. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:237: int bytesRead = in.read(actualOutput, 0, actualOutput.length); On 2014/12/02 19:01:50, mmenke wrote: > Should check that the next read (Using the same read operation) returns -1. Done. Great suggestion! I actually found a bug in CronetInputStream using your suggestion. https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:281: bytesRead = in.read(actualOutput, actualOutput.length - 10, 10); On 2014/12/02 19:01:49, mmenke wrote: > Should use different destination buffers for the two reads, to make sure we > don't write beyond the end on the first read. Done.
https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:95: assertEquals("GET", getResponseAsString(urlConnection)); On 2014/12/02 20:35:34, xunjieli wrote: > On 2014/12/02 19:01:49, mmenke wrote: > > What happens if we try to read from both implementations? I assume we should > > get exceptions with both? Should test that. > > I don't understand. I thought we are reading from the connection in the > getResponseAsString method. Sorry, misread the test...was thinking we were disconnecting after receiving the headers and part of the body (We should have a test that does that, btw, and then tries to read from the body and the headers after the disconnect). https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:113: fail(); I'm assuming fail doesn't just throw an exception, right? https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:116: } Should try to read from the body as well (Not using getResponseAsString - unclear if an exception will be thrown when getting the input stream, when trying to read from it, or neither..Test should make sure both fail at the same point)
One other comment https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/340001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:48: throw new IOException(e); On 2014/12/02 20:35:34, xunjieli wrote: > On 2014/12/02 19:01:49, mmenke wrote: > > This doesn't support resumption after an InterruptedException is thrown (If > > there's a quit message in the queue, for instance, the next attempt to run the > > loop quits instantly). Do other implementations allow more data to be read > > after an interrupted exception is thrown? > > OkHttp's implementation retries until the request succeeds > (http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa...). > However, if it encounters an non-recoverable IOException, okHttp just rethrows > it to the caller. > The reason that we have this InterruptedException here is that we are using a > LinkedBlockingQueue which blocks when there is no task to run. OkHttp's does not > have a blocking queue, so it does not need to handle InterruptedException > explicitly. > > I think it might be safe for now to just treat InterruptedException as an > unrecoverable IOException, since it might be the caller who explicitly > interrupts the thread. From the documentation > (https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html), > it does not mention anything about resuming. Also the Thread#resume() method is > deprecated (see > http://developer.android.com/reference/java/lang/Thread.html#resume()), so we > probably don't need to worry about resuming. We should throw an exception of some sort, then, on read attempts after the loop has thrown an exception.
I changed the CL to terminate the loop if waiting for task has been interrupted. I don't thinks we should care about waiting for mQueue.put(), since we are using a queue with Integer.MAX_VALUE capacity, so the put() call won't block. PTAL. thanks https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java (right): https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:113: fail(); On 2014/12/02 20:48:11, mmenke wrote: > I'm assuming fail doesn't just throw an exception, right? Yes, fail() just makes the test fails. https://codereview.chromium.org/725683002/diff/360001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLConnectionTest.java:116: } On 2014/12/02 20:48:11, mmenke wrote: > Should try to read from the body as well (Not using getResponseAsString - > unclear if an exception will be thrown when getting the input stream, when > trying to read from it, or neither..Test should make sure both fail at the same > point) Done.
I'm happy with this, just some comments on your latest changes. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:13: * {@link CronetHttpURLConnection}. Should mention the mLoopTerminated behavior either here, or in loop() https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:15: final class MessageLoop { Can we just make this implement Executor instead, and rename postTask to execute? (+ add @Override, of course). Won't have to wrap it in an Executor, then, too https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; Suggest explicitly setting these to false - relying on objects being initialized to null is one thing, but relying on primitive initialization strikes me as not great style. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; I don't think "terminated" is a great term here, since every time we exit loop(), the loop terminates. Maybe mTaskFailed? Also get rid of the terminated in comments. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; Maybe "It cannot be started..." -> "It cannot be safely started..., because there may be a pending quit task, which will result in exiting the next loop() call without doing anything." https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:54: } catch (InterruptedException e) { Can we test this? https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:58: } Out of paranoia, maybe catch any RunTimeException, set mLoopTerminated to true, and rethrow the exception unmodified as well?
Patchset #18 (id:400001) has been deleted
Thanks for the review! PTAL. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:13: * {@link CronetHttpURLConnection}. On 2014/12/03 16:21:01, mmenke wrote: > Should mention the mLoopTerminated behavior either here, or in loop() Done. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; On 2014/12/03 16:21:01, mmenke wrote: > I don't think "terminated" is a great term here, since every time we exit > loop(), the loop terminates. > > Maybe mTaskFailed? > > Also get rid of the terminated in comments. Done. I named it mWaitingInterrupted, as it is not really running the task failed, but waiting for task has been interrupted. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; On 2014/12/03 16:21:01, mmenke wrote: > Suggest explicitly setting these to false - relying on objects being initialized > to null is one thing, but relying on primitive initialization strikes me as not > great style. Done. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:23: private boolean mLoopTerminated; On 2014/12/03 16:21:01, mmenke wrote: > Maybe "It cannot be started..." -> "It cannot be safely started..., because > there may be a pending quit task, which will result in exiting the next loop() > call without doing anything." Done. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:54: } catch (InterruptedException e) { On 2014/12/03 16:21:01, mmenke wrote: > Can we test this? Done. https://codereview.chromium.org/725683002/diff/380001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:58: } On 2014/12/03 16:21:01, mmenke wrote: > Out of paranoia, maybe catch any RunTimeException, set mLoopTerminated to true, > and rethrow the exception unmodified as well But InterruptedException is not a runtime exception, it is a checked exception. Wrapping InterruptedException in a IOEXception is because the get* methods in HttpURLConnection only have throw IOException clauses. We need wrapping so we can surface the exception to callers right? I changed this part a bit to catch runtime exception, let me know if that looks better or I should revert. https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:65: } else if (e instanceof RuntimeException) { Not sure if this is what you have in mind..
https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:65: } else if (e instanceof RuntimeException) { On 2014/12/03 19:03:30, xunjieli wrote: > Not sure if this is what you have in mind.. I'd like to prevent running the loop again here, too - I suspect other implementations don't support trying to read against after an exception as well, but I'm sufficiently paranoid that I'd like to throw an exception on future calls to loop() (Or could look into what other implementations do, and copy them)
Thanks for the review. I think your concern is valid, we should prevent re-spinning the loop if a runtime exception occurred. PTAL https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:65: } else if (e instanceof RuntimeException) { On 2014/12/03 20:05:39, mmenke wrote: > On 2014/12/03 19:03:30, xunjieli wrote: > > Not sure if this is what you have in mind.. > > I'd like to prevent running the loop again here, too - I suspect other > implementations don't support trying to read against after an exception as well, > but I'm sufficiently paranoid that I'd like to throw an exception on future > calls to loop() (Or could look into what other implementations do, and copy > them) Done. I added a new test for this as well.
On 2014/12/03 20:46:53, xunjieli wrote: > Thanks for the review. I think your concern is valid, we should prevent > re-spinning the loop if a runtime exception occurred. PTAL > > https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java > (right): > > https://codereview.chromium.org/725683002/diff/420001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:65: > } else if (e instanceof RuntimeException) { > On 2014/12/03 20:05:39, mmenke wrote: > > On 2014/12/03 19:03:30, xunjieli wrote: > > > Not sure if this is what you have in mind.. > > > > I'd like to prevent running the loop again here, too - I suspect other > > implementations don't support trying to read against after an exception as > well, > > but I'm sufficiently paranoid that I'd like to throw an exception on future > > calls to loop() (Or could look into what other implementations do, and copy > > them) > > Done. I added a new test for this as well. Great! Want to take one more look through the code, but hope to sign off tomorrow.
LGTM
https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:26: public class CronetHttpURLConnection extends HttpURLConnection { So, what happens if they call a method that we don't override/implement, like getHeaderField*, getContent, getRequestProperties, etc? Should we at least document current limitations?
https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:26: public class CronetHttpURLConnection extends HttpURLConnection { On 2014/12/04 20:01:15, mef wrote: > So, what happens if they call a method that we don't override/implement, like > getHeaderField*, getContent, getRequestProperties, etc? Should we at least > document current limitations? They will get default implementations, a lot of them will return null. I am planning to add support for these methods in follow-up CLs. My goal is to match exactly what okhttp has implemented in HttpURLConnection.
I think it is a good start, and it is definitely step one out of many. :) https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:26: public class CronetHttpURLConnection extends HttpURLConnection { On 2014/12/04 20:06:50, xunjieli wrote: > On 2014/12/04 20:01:15, mef wrote: > > So, what happens if they call a method that we don't override/implement, like > > getHeaderField*, getContent, getRequestProperties, etc? Should we at least > > document current limitations? > > They will get default implementations, a lot of them will return null. I am > planning to add support for these methods in follow-up CLs. My goal is to match > exactly what okhttp has implemented in HttpURLConnection. Sounds good, thanks! https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:108: public InputStream getInputStream() throws IOException { Does default implementation of getContent() use this or do we have to implement it separately? https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:15: class CronetInputStream extends InputStream { nit: Given that CronetUrlRequestListener is a private inner class of CronetHTTPUrlConnection, should this also be an inner class? Does it have any use stand alone? https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:56: mLoopRunning = true; Can we get into the situation where loop() is called from different threads?
Yes, once this CL is in, I can proceed with the rest in parallel, and in small chunks, and hopefully that'll be easier to review :) https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:108: public InputStream getInputStream() throws IOException { On 2014/12/04 20:37:56, mef wrote: > Does default implementation of getContent() use this or do we have to implement > it separately? Yes, default implementation uses getInputStream() in getContent() (see http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/...). I don't think we need to override the default getContent(), since OkHttp does not do it. https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:15: class CronetInputStream extends InputStream { On 2014/12/04 20:37:56, mef wrote: > nit: Given that CronetUrlRequestListener is a private inner class of > CronetHTTPUrlConnection, should this also be an inner class? > Does it have any use stand alone? I don't think there's any use case for it to be used as a stand-alone. I make it package protected, so it cannot be used outside org.chromium.net.urlconnection. I was afraid that including this in CronetHttpURLConnection.java will make the file big and not easy to read. https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java (right): https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:56: mLoopRunning = true; On 2014/12/04 20:37:56, mef wrote: > Can we get into the situation where loop() is called from different threads? HttpURLConnection is not thread-safe, so clients should not share one connection among threads. As long as they are using the connection in one thread, loop() will always be called from the same thread.
On 2014/12/04 20:55:53, xunjieli wrote: > Yes, once this CL is in, I can proceed with the rest in parallel, and in small > chunks, and hopefully that'll be easier to review :) > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:108: > public InputStream getInputStream() throws IOException { > On 2014/12/04 20:37:56, mef wrote: > > Does default implementation of getContent() use this or do we have to > implement > > it separately? > > Yes, default implementation uses getInputStream() in getContent() (see > http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/...). > I don't think we need to override the default getContent(), since OkHttp does > not do it. > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java > (right): > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:15: > class CronetInputStream extends InputStream { > On 2014/12/04 20:37:56, mef wrote: > > nit: Given that CronetUrlRequestListener is a private inner class of > > CronetHTTPUrlConnection, should this also be an inner class? > > Does it have any use stand alone? > > I don't think there's any use case for it to be used as a stand-alone. I make it > package protected, so it cannot be used outside org.chromium.net.urlconnection. > I was afraid that including this in CronetHttpURLConnection.java will make the > file big and not easy to read. > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java > (right): > > https://codereview.chromium.org/725683002/diff/440001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java:56: > mLoopRunning = true; > On 2014/12/04 20:37:56, mef wrote: > > Can we get into the situation where loop() is called from different threads? > > HttpURLConnection is not thread-safe, so clients should not share one connection > among threads. As long as they are using the connection in one thread, loop() > will always be called from the same thread. sg, lgtm
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/725683002/440001
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/725683002/460001
Message was sent while issue was closed.
Committed patchset #20 (id:460001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/2a66c58f2d83717e43a50b39f75b3facc24e70f8 Cr-Commit-Position: refs/heads/master@{#306939} |
