Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
966743003:
[Cronet] Implement getOutputStream in CronetHttpURLConnection (Closed)
Description[Cronet] Implement getOutputStream in CronetHttpURLConnection
Implement post data with and without
setFixedLengthStreamingMode.
If setFixedLengthStreamMode is not set, we buffer whole
request body in memory. This is achieved by using
CronetBufferedOutputStream. Otherwise, we do not buffer
everything, and use CronetOutputStream.
Chunked mode is not yet supported, and will be added in a
follow-up CL, since this CL is already quite big for
reviewing.
BUG=398997
Committed: https://crrev.com/950576043d18caee3fc8bb28dfc432c3d6adcbda
Cr-Commit-Position: refs/heads/master@{#325593}
Patch Set 1 : #
Total comments: 22
Patch Set 2 : Address comments #Patch Set 3 : Added batch write #Patch Set 4 : Added tests #Patch Set 5 : Use member variable instead #Patch Set 6 : Use large data in buffered case too #
Total comments: 26
Patch Set 7 : Address Paul's and Misha's comments #
Total comments: 18
Patch Set 8 : Handle write out of content length #Patch Set 9 : Addressed Matt's comments #
Total comments: 39
Patch Set 10 : Changed connect() to not read response headers #Patch Set 11 : Use ByteBuffer instead of ByteArrayOutputStream in CronetBufferedOutputStream #
Total comments: 28
Patch Set 12 : Address Matt's comments #
Total comments: 4
Patch Set 13 : Test write after connect for fixed length streaming mode #
Total comments: 48
Patch Set 14 : Misha's comments #Patch Set 15 : Use an abstract class #Patch Set 16 : Rebased #Patch Set 17 : Remove unused #Messages
Total messages: 56 (15 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
I used ByteArrayOutputStream for the case where we have to buffer everything in memory. I still have not made the batch write operation for the case where we don't buffer everything in memory. Not very sure how to do that without writing out of bound of the internal buffer. Other than that, I have cleaned up the CL. Paul, could you take a look?
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
+ mmenke, mef. PTAL.
On 2015/03/02 16:21:38, xunjieli wrote: > + mmenke, mef. PTAL. Batch read/write might not be difficult as I thought it'd be. I have a temp CL at https://codereview.chromium.org/972213002/ to enable batch read for InputStream. I still need to add tests and verify it works. I am going to work on batch write in CronetOutputStream for this CL. Will update this email thread to let you know when it is ready (probably Friday). In the meanwhile, suggestions are greatly appreciated.
https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:24: private final CronetHttpURLConnection mConnection; I think this is dead if you agree with my logic on line 74. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:62: mConnection.connect(); Ditto with line 74. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:74: mConnection.connect(); If getOutputStream() starts the request, and we know getOutputStream() has already been called if we have a CronetBufferedOutputStream, then isn't this code unnecessary? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:84: return mBuffer.size(); If getOutputStream() starts the request, which AFAIK prompts the call to getLength(), won't that return 0 which isn't correct? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:53: private long mFixedContentLength = -1; Can this just be replaced with super.fixedContentLengthLong? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:202: boolean bufferRequestBody = false; dead? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:208: bufferRequestBody = true; dead? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:389: super.fixedContentLength = (int) Math.min(contentLength, Integer.MAX_VALUE); Should this set super.fixedContentLengthLong? or maybe just calling super.setFixedLengthStreamingMode() is safer? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:412: * Used by {@link CronetOutputStream.CronetUploadDataProvider} to wait for What is CronetOutputStream.CronetUploadDataProvider? I see no class with that name. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:20: // CronetUploadStream buffers up to this value and wait for UploadDataStream What is CronetUploadStream? Do you mean CronetUploadDataStream?
Thanks for the review! I have addressed the comments, but I haven't added the batch write yet. Going to work on it this afternoon. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:24: private final CronetHttpURLConnection mConnection; On 2015/03/04 16:44:10, pauljensen wrote: > I think this is dead if you agree with my logic on line 74. Sorry, getOutputStream() is not organized properly so it isn't very easy to see that startRequest() isn't invoked there when the OutputStream is of this type. If we choose to buffer the request body (using this class), we only start the request when the consumer initiates a connect action, or in the case when content-length is known, we start the request automatically when we reached that number of bytes (line 62 & 74). Since CronetBufferedOutputStream#getLength() is only invoked when we start the request, the result returned by getLength() is correct. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:62: mConnection.connect(); On 2015/03/04 16:44:10, pauljensen wrote: > Ditto with line 74. above. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:74: mConnection.connect(); On 2015/03/04 16:44:10, pauljensen wrote: > If getOutputStream() starts the request, and we know getOutputStream() has > already been called if we have a CronetBufferedOutputStream, then isn't this > code unnecessary? above. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:84: return mBuffer.size(); On 2015/03/04 16:44:10, pauljensen wrote: > If getOutputStream() starts the request, which AFAIK prompts the call to > getLength(), won't that return 0 which isn't correct? above. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:53: private long mFixedContentLength = -1; On 2015/03/04 16:44:11, pauljensen wrote: > Can this just be replaced with super.fixedContentLengthLong? Done. Didn't realize this field exists in 1.7. Was looking at an older version. Thanks for pointing out! Will it have any compatibility issue? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:202: boolean bufferRequestBody = false; On 2015/03/04 16:44:10, pauljensen wrote: > dead? Done. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:208: bufferRequestBody = true; On 2015/03/04 16:44:10, pauljensen wrote: > dead? Done. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:389: super.fixedContentLength = (int) Math.min(contentLength, Integer.MAX_VALUE); On 2015/03/04 16:44:11, pauljensen wrote: > Should this set super.fixedContentLengthLong? or maybe just calling > super.setFixedLengthStreamingMode() is safer? Done. this long version exists only since Jdk 1.7. Will it be compatible? https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:412: * Used by {@link CronetOutputStream.CronetUploadDataProvider} to wait for On 2015/03/04 16:44:11, pauljensen wrote: > What is CronetOutputStream.CronetUploadDataProvider? I see no class with that > name. Done. Sorry, forgot to update the comment. https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:20: // CronetUploadStream buffers up to this value and wait for UploadDataStream On 2015/03/04 16:44:11, pauljensen wrote: > What is CronetUploadStream? Do you mean CronetUploadDataStream? Done. Sorry, it should be CronetOutputStream.
https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:53: private long mFixedContentLength = -1; On 2015/03/05 17:43:11, xunjieli wrote: > On 2015/03/04 16:44:11, pauljensen wrote: > > Can this just be replaced with super.fixedContentLengthLong? > > Done. Didn't realize this field exists in 1.7. Was looking at an older version. > Thanks for pointing out! Will it have any compatibility issue? Looks like this long version is in API level 19 +. So it won't be compatible with ICS and JB. Should I revert?
https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:53: private long mFixedContentLength = -1; On 2015/03/05 18:28:22, xunjieli wrote: > On 2015/03/05 17:43:11, xunjieli wrote: > > On 2015/03/04 16:44:11, pauljensen wrote: > > > Can this just be replaced with super.fixedContentLengthLong? > > > > Done. Didn't realize this field exists in 1.7. Was looking at an older > version. > > Thanks for pointing out! Will it have any compatibility issue? > > Looks like this long version is in API level 19 +. So it won't be compatible > with ICS and JB. Should I revert? :( ya...
On 2015/03/06 13:07:43, pauljensen wrote: > https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java > (right): > > https://codereview.chromium.org/966743003/diff/20001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:53: > private long mFixedContentLength = -1; > On 2015/03/05 18:28:22, xunjieli wrote: > > On 2015/03/05 17:43:11, xunjieli wrote: > > > On 2015/03/04 16:44:11, pauljensen wrote: > > > > Can this just be replaced with super.fixedContentLengthLong? > > > > > > Done. Didn't realize this field exists in 1.7. Was looking at an older > > version. > > > Thanks for pointing out! Will it have any compatibility issue? > > > > Looks like this long version is in API level 19 +. So it won't be compatible > > with ICS and JB. Should I revert? > > :( ya... Done. Also added batch write and some tests. PTAL. Thanks!
https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:31: * Packaged protected constructor. nit: Package protected constructor. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:42: if (contentLength > Integer.MAX_VALUE) { what if it is negative, but not -1? https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:60: if (mBuffer.size() == mInitialContentLength) { what happens if app will write more than expected? https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:72: if (mBuffer.size() == mInitialContentLength) { When does it connect() if mInitialContentLength == -1? https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:78: @Override nit: Add comment that these methods are implementation of UploadDataProvider.
https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:247: // Start the request. Note that connect() is not used since Can we move this comment down so it's right before mRequest.start()? I think it makes more sense there. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:248: // connect() blocks until headers are received. I think it's a little odd to mention using connect() in a function that implements connect(). Part of me wonders if we should remove the readResponse logic and change connect() to: if (connected) checkHasResponse(); startRequest(); // Blocks until onResponseStarted or onFailed is called. mMessageLoop.loop(); checkHasResponse(); or perhaps even simpler: if (!connected) { startRequest(); // Blocks until onResponseStarted or onFailed is called. mMessageLoop.loop(); } checkHasResponse(); https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:379: @Override Can we be overriding this if it is not present on prior API versions? https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:391: super.fixedContentLength = (int) Math.min(contentLength, Integer.MAX_VALUE); I still feel like this should call super.setFixedLengthStreamingMode(), otherwise the super's long version won't be updated properly. Part of me wonders if we should be overriding this at all, and instead replace uses of mFixedContentLength with accesses to super.fixedContentLength? Perhaps we'll have to use some reflection to see if the parent has the long field. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:17: * This implementation does not buffer entire request body in memory. What threads do we expect this to be called from? I see problems if read() and write() are called simultaneously. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:22: // FIXME: confirm this number is what we want to buffer internally. can we either resolve this FIXME now or change it to a TODO if it can wait? https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:58: byteBuffer.put(mBuffer); how do we know this won't overflow? can we add a comment to the fact https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:96: int toSent = count; how about renaming toSent to toSend
Thank you for the valuable reviews! I need to work on the issues that have been brought up. But I am doing interview training this afternoon (1-5pm), so I'll probably get back to you tomorrow. Meanwhile, if you have time, you could review my other CL (batch read) instead. Thanks so much!
Patchset #7 (id:140001) has been deleted
Thanks for the valuable reviews, which have caught many bugs! I have addressed all comments and added tests. PTAL. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:31: * Packaged protected constructor. On 2015/03/10 16:48:22, mef wrote: > nit: Package protected constructor. Done. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:42: if (contentLength > Integer.MAX_VALUE) { On 2015/03/10 16:48:22, mef wrote: > what if it is negative, but not -1? Done. Right, we should handle that case. To make argument checking clearer, I have separate the constructor into two. One is for when content length is known, the other is when content length is unknown. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:60: if (mBuffer.size() == mInitialContentLength) { On 2015/03/10 16:48:22, mef wrote: > what happens if app will write more than expected? Done. Good catch, it should fail with a java.net.ProtocolException. I have added a test to verify against the default behavior. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:72: if (mBuffer.size() == mInitialContentLength) { On 2015/03/10 16:48:22, mef wrote: > When does it connect() if mInitialContentLength == -1? The embedder calls connect() or any function that implicitly calls connect() (e.g/ getHttpStatusCode, getInputStream) https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:78: @Override On 2015/03/10 16:48:22, mef wrote: > nit: Add comment that these methods are implementation of UploadDataProvider. Done. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:247: // Start the request. Note that connect() is not used since On 2015/03/10 17:31:05, pauljensen wrote: > Can we move this comment down so it's right before mRequest.start()? I think it > makes more sense there. Done. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:248: // connect() blocks until headers are received. On 2015/03/10 17:31:05, pauljensen wrote: > I think it's a little odd to mention using connect() in a function that > implements connect(). Part of me wonders if we should remove the readResponse > logic and change connect() to: > if (connected) checkHasResponse(); > startRequest(); > // Blocks until onResponseStarted or onFailed is called. > mMessageLoop.loop(); > checkHasResponse(); > or perhaps even simpler: > if (!connected) { > startRequest(); > // Blocks until onResponseStarted or onFailed is called. > mMessageLoop.loop(); > } > checkHasResponse(); Done. Good idea! I was thinking connect() shouldn't block when I did the readResponse logic, but ended up not finding a way around it. Thanks! https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:379: @Override On 2015/03/10 17:31:05, pauljensen wrote: > Can we be overriding this if it is not present on prior API versions? There's no warning, so I guess it's not prohibited. I have adopted your reflection suggestion. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:391: super.fixedContentLength = (int) Math.min(contentLength, Integer.MAX_VALUE); On 2015/03/10 17:31:05, pauljensen wrote: > I still feel like this should call super.setFixedLengthStreamingMode(), > otherwise the super's long version won't be updated properly. Part of me > wonders if we should be overriding this at all, and instead replace uses of > mFixedContentLength with accesses to super.fixedContentLength? Perhaps we'll > have to use some reflection to see if the parent has the long field. Done. Thanks for the suggestion! https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:17: * This implementation does not buffer entire request body in memory. On 2015/03/10 17:31:06, pauljensen wrote: > What threads do we expect this to be called from? I see problems if read() and > write() are called simultaneously. I guess I was assuming that writes happen on the thread that the request is created on. So read() and write() won't be called at the same time. This is not clear, so I have edited the docs too. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:22: // FIXME: confirm this number is what we want to buffer internally. On 2015/03/10 17:31:06, pauljensen wrote: > can we either resolve this FIXME now or change it to a TODO if it can wait? Done. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:58: byteBuffer.put(mBuffer); On 2015/03/10 17:31:06, pauljensen wrote: > how do we know this won't overflow? can we add a comment to the fact Sorry, I think there's a risk of overflowing. Done. I have added a test too. https://codereview.chromium.org/966743003/diff/120001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:96: int toSent = count; On 2015/03/10 17:31:06, pauljensen wrote: > how about renaming toSent to toSend Done.
Realized that I didn't handle to case where we write more bytes than content length in CronetOutputStream. I have made edits and added two more tests.
Not a full review. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:114: return mBuffer.size(); Wait...we're requiring the entire body be passed in to us before the request is started? I can't find any mention of that in the docs. Shouldn't we wait until close() has been called in that case? https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:206: // in API 19) is inherited. I don't suppose we can just make our own implementation of setFixedLengthStreamingMode (long and short versions)? Admittedly, we couldn't mark the second one with @Override, it wouldn't be on older API versions, but it would allow us to test the functionality, regardless of API version. I don't know Java well enough to know if it would complain about having one overloaded member act as an override, and the other not. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:211: (long) (parent.getField("fixedContentLengthLong").get(this)); Instead of the cast, there's a getLong method. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:222: startRequest(); This seems overly complicated. Why not: long contentLength = fixedContentLength; // Try to get superFixedContentLengthLong here, and overwrite contentLength with it. // API docs are unclear if we need to check if superFixedContentLengthLong is -1 // before overwrite or not. if (contentLength != -1) { ... } https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:233: mOutputStream = new CronetBufferedOutputStream(this, lengthParsed); Should we do something if we can't parse the length as a long, or it's < 0? https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:26: @VisibleForTesting What does this do? I don't see any of our tests using the value below, and since it's private, I assume it can't be accessed. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:27: private static int sDefaultBufferLength = 2048; This should probably be 32k
Thanks for the review! https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:114: return mBuffer.size(); On 2015/03/13 14:51:11, mmenke wrote: > Wait...we're requiring the entire body be passed in to us before the request is > started? I can't find any mention of that in the docs. Yes, we require the entire body to be passed to us, or rather, buffered in memory, when setFixedLengthStreamingMode() is not used. The relevant info is in "Posting content" section of http://developer.android.com/reference/java/net/HttpURLConnection.html. > Shouldn't we wait until > close() has been called in that case? We should probably implement close() to check whether enough data has been received. But we have to set headers and start the request when connect() action is initiated by the user (either explicitly or implicitly through getInputStream/getStatusCode). https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:206: // in API 19) is inherited. On 2015/03/13 14:51:12, mmenke wrote: > I don't suppose we can just make our own implementation of > setFixedLengthStreamingMode (long and short versions)? Admittedly, we couldn't > mark the second one with @Override, it wouldn't be on older API versions, but it > would allow us to test the functionality, regardless of API version. > I adopted this approach earlier, but changed after Paul suggested reflection, which I think is slightly cleaner. FYI, okhttp does use the override-with-own-implementation approach (https://github.com/square/okhttp/blob/master/okhttp-urlconnection/src/main/ja...) > I don't know Java well enough to know if it would complain about having one > overloaded member act as an override, and the other not. It won't complain. But it looks like "@Override" is removed in aosp http://androidxref.com/4.4.3_r1.1/xref/external/okhttp/src/main/java/com/squa.... https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:211: (long) (parent.getField("fixedContentLengthLong").get(this)); On 2015/03/13 14:51:11, mmenke wrote: > Instead of the cast, there's a getLong method. Done. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:222: startRequest(); On 2015/03/13 14:51:11, mmenke wrote: > This seems overly complicated. > > Why not: > > long contentLength = fixedContentLength; > // Try to get superFixedContentLengthLong here, and overwrite contentLength with > it. > // API docs are unclear if we need to check if superFixedContentLengthLong is -1 > // before overwrite or not. > if (contentLength != -1) { > ... > } Done. Added check before overriding. Looking at the official implementation, the int version and the long version are completely separated. If the user only sets the int version, long version is -1, so we wouldn't want to override. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:233: mOutputStream = new CronetBufferedOutputStream(this, lengthParsed); On 2015/03/13 14:51:11, mmenke wrote: > Should we do something if we can't parse the length as a long, or it's < 0? If there's a parse error, an runtime exception (NumberFormatException) will be propagated to the user. If it < 0, the CronetBufferedOutputStream constructor will complain. I am not sure if we can do anything else. Maybe we can catch NumberFormatException and IllegalArgumentException and make it into an IOException? Not sure if that's a good idea. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:26: @VisibleForTesting On 2015/03/13 14:51:12, mmenke wrote: > What does this do? I don't see any of our tests using the value below, and > since it's private, I assume it can't be accessed. The docs on this flag says "Annotation used to mark code that has wider visibility or present for testing code", I put it here to make it clear why this static field isn't final. There is a public setter for this variable. This is to test the "byteBuffer.remaining() < mBuffer.position()" case below (unit test is in CronetOutputStreamTest.java). https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:27: private static int sDefaultBufferLength = 2048; On 2015/03/13 14:51:12, mmenke wrote: > This should probably be 32k But 32k is larger than all the read buffers used in QUIC(14520), SPDY(2852) and normal stream(16384). Why do we use 32k? Should we use 16384 (1 << 14) instead?
Egads! This CL was sent out for review almost a month ago. Given the state of the original CL, that's way too long. Sorry the ball got completely dropped here. I may not have time to do a (Hopefully final/near-final) round of review today, but if not, definitely tomorrow.
Thanks Matt! Don't worry too much about it. The only current consumer (Paul's integration) is getting delayed. I have other things to work in the meanwhile :) On Tue, Mar 24, 2015 at 2:04 PM, <mmenke@chromium.org> wrote: > Egads! This CL was sent out for review almost a month ago. Given the > state of > the original CL, that's way too long. Sorry the ball got completely > dropped > here. I may not have time to do a (Hopefully final/near-final) round of > review > today, but if not, definitely tomorrow. > > https://codereview.chromium.org/966743003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I meant to say that Paul's integration is delayed for other reasons, IIUC. So no one depends on getting this shipped right now. So take your time in reviewing! Thanks again. On Tue, Mar 24, 2015 at 2:15 PM, Helen Li <xunjieli@chromium.org> wrote: > Thanks Matt! Don't worry too much about it. The only current consumer > (Paul's integration) is getting delayed. I have other things to work in the > meanwhile :) > > > > On Tue, Mar 24, 2015 at 2:04 PM, <mmenke@chromium.org> wrote: > >> Egads! This CL was sent out for review almost a month ago. Given the >> state of >> the original CL, that's way too long. Sorry the ball got completely >> dropped >> here. I may not have time to do a (Hopefully final/near-final) round of >> review >> today, but if not, definitely tomorrow. >> >> https://codereview.chromium.org/966743003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:114: return mBuffer.size(); On 2015/03/13 19:26:40, xunjieli wrote: > On 2015/03/13 14:51:11, mmenke wrote: > > Wait...we're requiring the entire body be passed in to us before the request > is > > started? I can't find any mention of that in the docs. > > Yes, we require the entire body to be passed to us, or rather, buffered in > memory, when setFixedLengthStreamingMode() is not used. > The relevant info is in "Posting content" section of > http://developer.android.com/reference/java/net/HttpURLConnection.html. So when uploading data, calling connect() breaks things? Unless you've passed the entire content already, regardless of whether you're using fixed length streaming mode or not. > > Shouldn't we wait until > > close() has been called in that case? > > We should probably implement close() to check whether enough data has been > received. But we have to set headers and start the request when connect() action > is initiated by the user (either explicitly or implicitly through > getInputStream/getStatusCode). > https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:233: mOutputStream = new CronetBufferedOutputStream(this, lengthParsed); On 2015/03/13 19:26:40, xunjieli wrote: > On 2015/03/13 14:51:11, mmenke wrote: > > Should we do something if we can't parse the length as a long, or it's < 0? > > If there's a parse error, an runtime exception (NumberFormatException) will be > propagated to the user. If it < 0, the CronetBufferedOutputStream constructor > will complain. > I am not sure if we can do anything else. Maybe we can catch > NumberFormatException and IllegalArgumentException and make it into an > IOException? Not sure if that's a good idea. As long as things will fail, I'm fine with the current code. https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:27: private static int sDefaultBufferLength = 2048; On 2015/03/13 19:26:40, xunjieli wrote: > On 2015/03/13 14:51:12, mmenke wrote: > > This should probably be 32k > > But 32k is larger than all the read buffers used in QUIC(14520), SPDY(2852) and > normal stream(16384). Why do we use 32k? Should we use 16384 (1 << 14) instead? You're right. We generally read from URLRequests with a 32k buffer, but use a smaller buffer for read data to be uploaded. That seems a little weird. Wonder if it's worth testing upload buffer sizes, C++-side. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:20: * or {@link CronetHttpURLConnection#setChunkedStreamingMode} is set. nit: or -> nor https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:68: // Bufferring without knowing content-length. nit: Buffering. Last letter often isn't doubled for words with more than on syllable. Don't ask me why. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:75: checkNotExceedContentLength(1); Should we also check that the connection hasn't started yet? We don't have any protection against adding data after connection when mInitialContentLength is -1. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:126: byteBuffer.put(mBuffer.toByteArray(), mBytesConsumed, toConsume); The docs for toByteArray says it's a copy of the buffer's memory, so this may copy the entire array on every read, which is really inefficient. We could just cache the array, but that's still inefficient... We could use byteBuffer.slice(), though, which creates another view of the same buffer, and then set the position() and limit() of the slice appropriately. Or perhaps better, just update them for mBuffer itself for the copy, and then restore the values as needed. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: checkHasResponse(); I'm still hazy on this API. If you're uploading, you aren't allowed to call connect(), unless you're already done passing along the entire body for upload? In the CronetOutputStream case, the checkHasResponse() line will fail. In the other case, things will also go badly. Should we explicitly check for that case? https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:213: // content-length bytes are received, or when an nit: an -> a https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:276: // connect() blocks until headers are received. This comment about connect is a little weird, since this is now the guts of connect(). https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:21: * {@link #mConnection} is created. This should mention "fixedStreamingMode" or "fixed streaming mode" and the lack of rewind support. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:23: final class CronetOutputStream extends OutputStream implements UploadDataProvider { Maybe rename to something like CronetFixedModeOutputStream, to align a bit more with "fixedStreamingMode", and sound less general. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:122: Runnable readTask = new Runnable() { Can't we just inline this instead of making it a task? We're running it with run() rather than posting it as a task, anyways, because we're already running on the CronetHttpURLConnection's executor loop. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:155: uploadDataSink.onRewindError(null); Passing a null exception along seems like a bad idea. There may be checks elsewhere that a null exception means no exception, as opposed to...erm, an exception that just was null. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:131: public void testPostWithZeroContentLength() throws Exception { Maybe a zero length upload without a content length, too? https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:181: out.write(UPLOAD_DATA); Suggest a test like this that does two writes. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:236: assertTrue(Arrays.equals(UPLOAD_DATA, byteBuffer.array())); Rather than doing all this manually, can we just use a method-preserving redirect? I assume the default implementation can handle that, too, so can also run it with both implementations. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:101: } Should probably call disconnect at the end of all these tests - not currently needed on some of them, currently, but seems like a good idea (The underlying API doesn't guarantee disconnection on error, does it?) https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:119: out.write(UPLOAD_DATA[i]); Suggest modifying one of these two tests so we actually upload something before the failure (And will then need to call disconnect(), I believe) https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:172: public void testFixedLengthStreamingModeLargeData() throws Exception { Maybe add a test like this where we just do one massive write? This test won't end up writing more than the buffer size (2048 bytes) in one go, which is something we should test. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:184: connection.setFixedLengthStreamingMode(UPLOAD_DATA.length * REPEAT_COUNT); optional: Suggest a method GetLargeData(), and using it here and in the next test, to make things a little simpler https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:266: } Maybe test a POST-preserving redirect, which should fail.
Thanks for the review! Sorry it took me so long. The major change that I made is that connect() no longer blocks until response headers, connect() now only starts the request and sends request headers. I think this should be the way to go. PTAL. Thanks! https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/160001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:233: mOutputStream = new CronetBufferedOutputStream(this, lengthParsed); On 2015/03/25 18:34:57, mmenke wrote: > On 2015/03/13 19:26:40, xunjieli wrote: > > On 2015/03/13 14:51:11, mmenke wrote: > > > Should we do something if we can't parse the length as a long, or it's < 0? > > > > If there's a parse error, an runtime exception (NumberFormatException) will be > > propagated to the user. If it < 0, the CronetBufferedOutputStream constructor > > will complain. > > I am not sure if we can do anything else. Maybe we can catch > > NumberFormatException and IllegalArgumentException and make it into an > > IOException? Not sure if that's a good idea. > > As long as things will fail, I'm fine with the current code. Acknowledged. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:20: * or {@link CronetHttpURLConnection#setChunkedStreamingMode} is set. On 2015/03/25 18:34:57, mmenke wrote: > nit: or -> nor Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:68: // Bufferring without knowing content-length. On 2015/03/25 18:34:57, mmenke wrote: > nit: Buffering. Last letter often isn't doubled for words with more than on > syllable. Don't ask me why. Done. :) https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:75: checkNotExceedContentLength(1); On 2015/03/25 18:34:57, mmenke wrote: > Should we also check that the connection hasn't started yet? We don't have any > protection against adding data after connection when mInitialContentLength is > -1. Strangely the default implementation allows writing after connect() when initial content length is not specified. Not sure whether that's working as intended or a bug. We can't really match that behavior easily, so I added a java side check. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:126: byteBuffer.put(mBuffer.toByteArray(), mBytesConsumed, toConsume); On 2015/03/25 18:34:57, mmenke wrote: > The docs for toByteArray says it's a copy of the buffer's memory, so this may > copy the entire array on every read, which is really inefficient. > > We could just cache the array, but that's still inefficient... > > We could use byteBuffer.slice(), though, which creates another view of the same > buffer, and then set the position() and limit() of the slice appropriately. > > Or perhaps better, just update them for mBuffer itself for the copy, and then > restore the values as needed. Hmm.. Not sure if I understand, mBuffer is an ByteArrayOutputStream (used it because it grows automatically). It does not support updating position and limit. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: checkHasResponse(); On 2015/03/25 18:34:57, mmenke wrote: > I'm still hazy on this API. If you're uploading, you aren't allowed to call > connect(), unless you're already done passing along the entire body for upload? > In the CronetOutputStream case, the checkHasResponse() line will fail. In the > other case, things will also go badly. > > Should we explicitly check for that case? Looking at Okhttp's implementation, connect() does not read the response, so we probably should not block in connect(). Updated the implementation. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:213: // content-length bytes are received, or when an On 2015/03/25 18:34:57, mmenke wrote: > nit: an -> a Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:276: // connect() blocks until headers are received. On 2015/03/25 18:34:57, mmenke wrote: > This comment about connect is a little weird, since this is now the guts of > connect(). Done. Ah.. sorry should have removed it. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:21: * {@link #mConnection} is created. On 2015/03/25 18:34:57, mmenke wrote: > This should mention "fixedStreamingMode" or "fixed streaming mode" and the lack > of rewind support. Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:23: final class CronetOutputStream extends OutputStream implements UploadDataProvider { On 2015/03/25 18:34:57, mmenke wrote: > Maybe rename to something like CronetFixedModeOutputStream, to align a bit more > with "fixedStreamingMode", and sound less general. Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:122: Runnable readTask = new Runnable() { On 2015/03/25 18:34:57, mmenke wrote: > Can't we just inline this instead of making it a task? We're running it with > run() rather than posting it as a task, anyways, because we're already running > on the CronetHttpURLConnection's executor loop. Done. I ran into problem with loop() being invoked in connect(), so I had this readTask (which I postponed after quit message is posted). After making connect() non-blocking, I am able to inline it. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java:155: uploadDataSink.onRewindError(null); On 2015/03/25 18:34:57, mmenke wrote: > Passing a null exception along seems like a bad idea. There may be checks > elsewhere that a null exception means no exception, as opposed to...erm, an > exception that just was null. Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:131: public void testPostWithZeroContentLength() throws Exception { On 2015/03/25 18:34:57, mmenke wrote: > Maybe a zero length upload without a content length, too? Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:181: out.write(UPLOAD_DATA); On 2015/03/25 18:34:57, mmenke wrote: > Suggest a test like this that does two writes. Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:236: assertTrue(Arrays.equals(UPLOAD_DATA, byteBuffer.array())); On 2015/03/25 18:34:57, mmenke wrote: > Rather than doing all this manually, can we just use a method-preserving > redirect? I assume the default implementation can handle that, too, so can also > run it with both implementations. Done. Good idea! But the default implementation returns empty body on redirect, so I am still using OnlyRunCronetHttpURLConnection. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:101: } On 2015/03/25 18:34:58, mmenke wrote: > Should probably call disconnect at the end of all these tests - not currently > needed on some of them, currently, but seems like a good idea (The underlying > API doesn't guarantee disconnection on error, does it?) Done. Yes you are right. The underlying API makes no guarantee that it will disconnect on error. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:119: out.write(UPLOAD_DATA[i]); On 2015/03/25 18:34:58, mmenke wrote: > Suggest modifying one of these two tests so we actually upload something before > the failure (And will then need to call disconnect(), I believe) Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:172: public void testFixedLengthStreamingModeLargeData() throws Exception { On 2015/03/25 18:34:58, mmenke wrote: > Maybe add a test like this where we just do one massive write? This test won't > end up writing more than the buffer size (2048 bytes) in one go, which is > something we should test. Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:184: connection.setFixedLengthStreamingMode(UPLOAD_DATA.length * REPEAT_COUNT); On 2015/03/25 18:34:57, mmenke wrote: > optional: Suggest a method GetLargeData(), and using it here and in the next > test, to make things a little simpler Done. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetOutputStreamTest.java:266: } On 2015/03/25 18:34:58, mmenke wrote: > Maybe test a POST-preserving redirect, which should fail. Done.
Quick response. https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:126: byteBuffer.put(mBuffer.toByteArray(), mBytesConsumed, toConsume); On 2015/03/27 17:46:44, xunjieli wrote: > On 2015/03/25 18:34:57, mmenke wrote: > > The docs for toByteArray says it's a copy of the buffer's memory, so this may > > copy the entire array on every read, which is really inefficient. > > > > We could just cache the array, but that's still inefficient... > > > > We could use byteBuffer.slice(), though, which creates another view of the > same > > buffer, and then set the position() and limit() of the slice appropriately. > > > > Or perhaps better, just update them for mBuffer itself for the copy, and then > > restore the values as needed. > > Hmm.. Not sure if I understand, mBuffer is an ByteArrayOutputStream (used it > because it grows automatically). It does not support updating position and > limit. Ahh, right. This is still a problem, since ByteArrayOutputStream.toByteArray() returns a copy, calling it every time is expensive. Can we switch to a ByteBuffer, and then just set the position and limit as needed?
Patchset #11 (id:240001) has been deleted
On 2015/03/27 17:54:10, mmenke wrote: > Quick response. > > https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... > File > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java > (right): > > https://codereview.chromium.org/966743003/diff/200001/components/cronet/andro... > components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:126: > byteBuffer.put(mBuffer.toByteArray(), mBytesConsumed, toConsume); > On 2015/03/27 17:46:44, xunjieli wrote: > > On 2015/03/25 18:34:57, mmenke wrote: > > > The docs for toByteArray says it's a copy of the buffer's memory, so this > may > > > copy the entire array on every read, which is really inefficient. > > > > > > We could just cache the array, but that's still inefficient... > > > > > > We could use byteBuffer.slice(), though, which creates another view of the > > same > > > buffer, and then set the position() and limit() of the slice appropriately. > > > > > > Or perhaps better, just update them for mBuffer itself for the copy, and > then > > > restore the values as needed. > > > > Hmm.. Not sure if I understand, mBuffer is an ByteArrayOutputStream (used it > > because it grows automatically). It does not support updating position and > > limit. > > Ahh, right. This is still a problem, since ByteArrayOutputStream.toByteArray() > returns a copy, calling it every time is expensive. > > Can we switch to a ByteBuffer, and then just set the position and limit as > needed? Done. Agree, ByteBuffer does seem to be the better option. Thanks!
Looks good, just minor suggestions. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:75: } optional: Suggest moving this surrounding logic into growInternalBuffer, since it's duplicated in both write functions. And maybe rename it (ensureCanWrite or something? Then the check that we're not connected could be moved here, too - it's a little weird in "checkNotExceedContentLength") https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:117: private void checkNotExceedContentLength(int numBytes) throws ProtocolException { nit: checkContentLengthNotExceeded? Or could just inline it, if you follow my suggestion about changing growInternalBuffer. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:149: } Can we move this to setConnected, and get rid of mFirstReadInitiated? If we need mFirstReadInitiated, I'm fine with keeping it, just not a big fan of variables that aren't actually needed. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:72: startRequest(); I'm fine with this, but what was the reason for making connect no longer go what getResponse does now? https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:82: if (connected) { Not really related to this CL, but should we set connected to false here? Also, should we allow multiple disconnect calls? calling mRequest.cancel() and mInputStream.close multiple times seems like a bad idea, if we should support it. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:258: } Should we throw here or in setConnected if we're using fixed length mode and not enough data has been appended? (And should we have a test for that?) https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:274: connected = true; optional nit: Suggest either putting this just before the call to start(), or just before the doOutput check. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:47: public void testPostAfterConnectionMade() throws Exception { nit: Maybe testGetOutputStreamAfter...? https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:60: } Should we have a similar test where we get the output stream before reading from a request, but only try to append data afterwards? https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:166: // Make sure the server echos back empty body for both implementation. nit: echoes (I'm not sure if echos is a valid alternate spelling when used as a verb - for a noun, it is) https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:193: while (totalBytesWritten < largeData.length) { This loop should mirror testPostWithContentLength's. Since we also have OneMassiveWrite variants of both, I'm not too concerned which one you use - maybe this one, and start with 683, so first write will fit in buffer, but next will just exceed it? Up to you. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:54: throws Exception { Maybe a test where we try to connect or read a response without writing the entire body? https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:187: while (totalBytesWritten < largeData.length) { Again, suggest having this match the other two. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:241: public void testMassiveWrite() throws Exception { OneMassiveWrite, to match the tests in the other file?
Patchset #12 (id:280001) has been deleted
Thanks for the review! I have address all comments. PTAL. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:75: } On 2015/03/31 18:19:32, mmenke wrote: > optional: Suggest moving this surrounding logic into growInternalBuffer, since > it's duplicated in both write functions. And maybe rename it (ensureCanWrite or > something? Then the check that we're not connected could be moved here, too - > it's a little weird in "checkNotExceedContentLength") Done. You are totally right! that does make the code cleaner. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:117: private void checkNotExceedContentLength(int numBytes) throws ProtocolException { On 2015/03/31 18:19:32, mmenke wrote: > nit: checkContentLengthNotExceeded? Or could just inline it, if you follow my > suggestion about changing growInternalBuffer. Done.thanks! https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:149: } On 2015/03/31 18:19:32, mmenke wrote: > Can we move this to setConnected, and get rid of mFirstReadInitiated? If we > need mFirstReadInitiated, I'm fine with keeping it, just not a big fan of > variables that aren't actually needed. Done. Good idea! https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:72: startRequest(); On 2015/03/31 18:19:32, mmenke wrote: > I'm fine with this, but what was the reason for making connect no longer go what > getResponse does now? The message loop is shared with UploadDataStream. If we spin up message loop now, UploadDataStream will takes it as a signal to do reads, but the client might not be ready. So I decided to not spin the message loop here, and delay it when it is actually needed (in get*() methods). https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:82: if (connected) { On 2015/03/31 18:19:32, mmenke wrote: > Not really related to this CL, but should we set connected to false here? Also, > should we allow multiple disconnect calls? calling mRequest.cancel() and > mInputStream.close multiple times seems like a bad idea, if we should support > it. According to the API doc, "connected" indicates whether there's been a connect attempt. We should not set it back to false. We probably can test to see if mInputStream == null before we cancel mRequest, so we don't cancel the request multiple times. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:258: } On 2015/03/31 18:19:32, mmenke wrote: > Should we throw here or in setConnected if we're using fixed length mode and not > enough data has been appended? (And should we have a test for that?) Done. Good catch! That's an important case that that i've missed out. thanks. But I cannot add the check here, since startRequest() is called right away for fixed length mode (before consumer start writing). I have added the check when we try to read response. Also added the check for the CronetBufferedOutputStream. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:274: connected = true; On 2015/03/31 18:19:32, mmenke wrote: > optional nit: Suggest either putting this just before the call to start(), or > just before the doOutput check. Done. Moved to start() call, since addRequestProperty needs connected to be false. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:47: public void testPostAfterConnectionMade() throws Exception { On 2015/03/31 18:19:32, mmenke wrote: > nit: Maybe testGetOutputStreamAfter...? Done. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:60: } On 2015/03/31 18:19:32, mmenke wrote: > Should we have a similar test where we get the output stream before reading from > a request, but only try to append data afterwards? Done. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:166: // Make sure the server echos back empty body for both implementation. On 2015/03/31 18:19:32, mmenke wrote: > nit: echoes (I'm not sure if echos is a valid alternate spelling when used as a > verb - for a noun, it is) Done. TIL "echoes" :) https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetBufferedOutputStreamTest.java:193: while (totalBytesWritten < largeData.length) { On 2015/03/31 18:19:32, mmenke wrote: > This loop should mirror testPostWithContentLength's. Since we also have > OneMassiveWrite variants of both, I'm not too concerned which one you use - > maybe this one, and start with 683, so first write will fit in buffer, but next > will just exceed it? Up to you. Done. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:54: throws Exception { On 2015/03/31 18:19:32, mmenke wrote: > Maybe a test where we try to connect or read a response without writing the > entire body? Done. Good idea! https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:187: while (totalBytesWritten < largeData.length) { On 2015/03/31 18:19:32, mmenke wrote: > Again, suggest having this match the other two. Done. https://codereview.chromium.org/966743003/diff/260001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:241: public void testMassiveWrite() throws Exception { On 2015/03/31 18:19:32, mmenke wrote: > OneMassiveWrite, to match the tests in the other file? Done.
https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:504: } Should this be before the mHasResponse check? https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:53: public void testFixedLengthStreamingModeZeroContentLength() Didn't you say that with fix length streaming mode, connect() may be called before we're passed any of the body? Should we have a test for that?
Running my old tests, I found that non-blocking connect() has a limitation. The default implementation reports errors like (host unreachable, host name unresolvable) in connect() calls. However, since now Cronet's connect() call does not spin up the message loop, Cronet does not know about the error until it actually starts to read response. See CronetHttpURLCOnnectionTest#testServerNotAvailable, #testBadIP, #testBadHostname. I guess we can rework on the design, but I am not sure additional code complexity is worth tackling this limitation, since the consumer will eventually know about the errors when they start reading the response. Any thought? https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:504: } On 2015/04/01 17:41:05, mmenke wrote: > Should this be before the mHasResponse check? Done. Yes you are right! https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/966743003/diff/300001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:53: public void testFixedLengthStreamingModeZeroContentLength() On 2015/04/01 17:41:05, mmenke wrote: > Didn't you say that with fix length streaming mode, connect() may be called > before we're passed any of the body? Should we have a test for that? Done.
On 2015/04/01 19:04:55, xunjieli wrote: > Running my old tests, I found that non-blocking connect() has a limitation. The > default implementation reports errors like (host unreachable, host name > unresolvable) in connect() calls. However, since now Cronet's connect() call > does not spin up the message loop, Cronet does not know about the error until it > actually starts to read response. See > CronetHttpURLCOnnectionTest#testServerNotAvailable, #testBadIP, > #testBadHostname. > > I guess we can rework on the design, but I am not sure additional code > complexity is worth tackling this limitation, since the consumer will eventually > know about the errors when they start reading the response. Any thought? We could just call getResponse in connect() if it's not an upload. Less consistent behavior for us between uploading and not, but easy enough to implement. I'm not terribly concerned about it, either way, though. LGTM
On 2015/04/01 19:13:33, mmenke wrote: > On 2015/04/01 19:04:55, xunjieli wrote: > > Running my old tests, I found that non-blocking connect() has a limitation. > The > > default implementation reports errors like (host unreachable, host name > > unresolvable) in connect() calls. However, since now Cronet's connect() call > > does not spin up the message loop, Cronet does not know about the error until > it > > actually starts to read response. See > > CronetHttpURLCOnnectionTest#testServerNotAvailable, #testBadIP, > > #testBadHostname. > > > > I guess we can rework on the design, but I am not sure additional code > > complexity is worth tackling this limitation, since the consumer will > eventually > > know about the errors when they start reading the response. Any thought? > > We could just call getResponse in connect() if it's not an upload. Less > consistent behavior for us between uploading and not, but easy enough to > implement. I'm not terribly concerned about it, either way, though. > > LGTM I have considered that, but decided against it. The reason is that for fixed length streaming mode, the default implementation reports this type of error (eg. connection refused) in getOutputStream() as well as in connect(). So it is a little hard to match the behavior (can't just check whether we are uploading or not in connect()). Thanks for the reviews!
Pinging Misha and Paul for review..
Sorry for the delay, I haven't looked at tests yet. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:40: throw new NullPointerException(); add error message? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:44: throw new IllegalStateException("Use setFixedLengthStreamingMode()" I would throw IllegalArgumentException here for consistency with check below, and considering that this is caller-provided argument. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:67: mBuffer = ByteBuffer.allocate(2048); Define 2048 as constant? While there, should it be like 32k? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:109: if (mInitialContentLength != -1 || mBuffer.limit() - mBuffer.position() > count) { Can we split it into 2 separate checks? Also maybe combine it with first 'if mInitialContentLength != -1', to explicitly handle buffer that shouldn't grow? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:130: return mBuffer.position(); hrm, this will be 0 if rewind() is called. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:138: int availableSpace = byteBuffer.capacity() - byteBuffer.position(); Maybe rename availableSpace => readSize? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:27: // TODO(xunjieli): figure out whether this default value should be changed. I'd suggest 32k. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:67: mBytesReceived++; Rename mBytesReceived => mBytesWritten? Not sure about java code style guide, but Chromium C++ suggests prefix ++ like in ++mBytesReceived; https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: void checkReceivedEnoughContent() throws IOException { not used? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:118: if (mContentLength != -1 && mBytesReceived + numBytes > mContentLength) { When mContentLength could == -1? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:143: mBuffer.compact(); moving memory is not cheap, is there a way to avoid it? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:204: // Start the request now since all headers can be sent. why is this true? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:215: long lengthParsed = Long.parseLong(length); what if it is over 2gb? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:262: ((CronetBufferedOutputStream) mOutputStream).setConnected(); can mOutputStream be a common base class like 'CronetOutputStream' that has 'setConnected' method, so you wouldn't need to cast? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:265: if (getRequestProperty("Content-Length") == null) { suggest string constant for "Content-Length" and "Content-Type". https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:497: ((CronetFixedModeOutputStream) mOutputStream).checkReceivedEnoughContent(); Maybe make checkReceivedEnoughContent a base class method as well? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:514: if (!mHasResponse) throw new IllegalStateException("No response."); isn't it a duplicate of mResponseInfo == null? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:147: "org.chromium.net.urlconnection")) { could you elaborate on this?
PTAL. Thanks! https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:40: throw new NullPointerException(); On 2015/04/06 16:14:54, mef wrote: > add error message? Done. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:44: throw new IllegalStateException("Use setFixedLengthStreamingMode()" On 2015/04/06 16:14:54, mef wrote: > I would throw IllegalArgumentException here for consistency with check below, > and considering that this is caller-provided argument. Done. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:67: mBuffer = ByteBuffer.allocate(2048); On 2015/04/06 16:14:54, mef wrote: > Define 2048 as constant? While there, should it be like 32k? Hmm.. 2048 isn't used anywhere else in this file, so I am not sure whether that should a constant. The value does not matter a whole lot, since it will grow eventually if bytes passed in don't fit. I don't think it should be 32k. QUIC upload buffer : 14520, SPDY = 2852, and Normal: 16384. 32k is larger than all of them. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:109: if (mInitialContentLength != -1 || mBuffer.limit() - mBuffer.position() > count) { On 2015/04/06 16:14:54, mef wrote: > Can we split it into 2 separate checks? > Also maybe combine it with first 'if mInitialContentLength != -1', to explicitly > handle buffer that shouldn't grow? Done. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:130: return mBuffer.position(); On 2015/04/06 16:14:54, mef wrote: > hrm, this will be 0 if rewind() is called. getLength() is called in CronetUploadDataStream's constructor (before rewind() is called), so it should not matter. I have tests (CronetBuffferedOutputStream#testRewind and #testRewindWithoutContentLength) to make sure it works. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:138: int availableSpace = byteBuffer.capacity() - byteBuffer.position(); On 2015/04/06 16:14:54, mef wrote: > Maybe rename availableSpace => readSize? It's actually not the read size, since the else clause writes the whole of mBuffer, so I'd keep it as the availableSpace of the byteBuffer. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:27: // TODO(xunjieli): figure out whether this default value should be changed. On 2015/04/06 16:14:54, mef wrote: > I'd suggest 32k. I don't think it should be 32k. QUIC uses a upload buffer of 14520, SPDY uses 2852, and normal stream uses 16384. 32k is bigger than all of them. I think we can probably use 16384, but I am not sure. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:67: mBytesReceived++; On 2015/04/06 16:14:54, mef wrote: > Rename mBytesReceived => mBytesWritten? > Not sure about java code style guide, but Chromium C++ suggests prefix ++ like > in ++mBytesReceived; Done. I believe the increment should be after the variable in Java. I haven't seen the other variant that much. Looks like the same goes to the java files in chromium: https://code.google.com/p/chromium/codesearch#search/&q=file:java$%20%22%2B%2... https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: void checkReceivedEnoughContent() throws IOException { On 2015/04/06 16:14:54, mef wrote: > not used? Used in CronetHttpURLConnection's startRequest() to make sure we have received enough content. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:118: if (mContentLength != -1 && mBytesReceived + numBytes > mContentLength) { On 2015/04/06 16:14:54, mef wrote: > When mContentLength could == -1? Done. Good catch! https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:143: mBuffer.compact(); On 2015/04/06 16:14:54, mef wrote: > moving memory is not cheap, is there a way to avoid it? I don't think it will be a problem if our buffer size is 2048, since it is smaller than all buffer sizes used in Chromium (QUIC: 14520, SPDY: 2852, Normal: 16384), the code will use the else clause. However, if we make the buffer size bigger (say 16k or 32k), we will run into this code path. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:204: // Start the request now since all headers can be sent. On 2015/04/06 16:14:55, mef wrote: > why is this true? Because the user sets fixedLengthStreamingMode, so we know we can set the content length header. For the case where fixedLengthStreamingMode is not set, the request headers should only be sent after all content has been buffered into memory. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:215: long lengthParsed = Long.parseLong(length); On 2015/04/06 16:14:54, mef wrote: > what if it is over 2gb? CronetBufferedOutputStream's constructor will throw an illegal argument exception. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:262: ((CronetBufferedOutputStream) mOutputStream).setConnected(); On 2015/04/06 16:14:54, mef wrote: > can mOutputStream be a common base class like 'CronetOutputStream' that has > 'setConnected' method, so you wouldn't need to cast? Only CronetBufferedOutputStream needs this method. For the case of CronetFixedModeOutputStream, request is started (connected) when the output stream is constructed. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:265: if (getRequestProperty("Content-Length") == null) { On 2015/04/06 16:14:55, mef wrote: > suggest string constant for "Content-Length" and "Content-Type". Done. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:497: ((CronetFixedModeOutputStream) mOutputStream).checkReceivedEnoughContent(); On 2015/04/06 16:14:54, mef wrote: > Maybe make checkReceivedEnoughContent a base class method as well? Only CronetFixedModeOutputStream needs this method. CronetBufferedOutputStream's check is in setConnected. These two classes already have a base class OutputStream. Java does not allow multiple inheritance. If we were to make a base class for these two, that base class has to implement methods in OutputStream. However these two classes shouldn't share the common write methods, for example. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:514: if (!mHasResponse) throw new IllegalStateException("No response."); On 2015/04/06 16:14:55, mef wrote: > isn't it a duplicate of mResponseInfo == null? Nope. mResponseInfo can be null when the request failed. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:147: "org.chromium.net.urlconnection")) { On 2015/04/06 16:14:55, mef wrote: > could you elaborate on this? I added two other test files (CronetFixedModeOutputStreamTest.java and CronetBufferedOutputStreamTest.java) which will use @OnlyRunCronetHttpURLConnection and @CompareDefaultWithCronet annotation. I changed "CronetHttpURLConnectionTest.class.getName()" to apply the parsing of the annotations to all files in org.chromium.net.urlconnection test package.
https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:67: mBuffer = ByteBuffer.allocate(2048); On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > Define 2048 as constant? While there, should it be like 32k? > > Hmm.. 2048 isn't used anywhere else in this file, so I am not sure whether that > should a constant. The value does not matter a whole lot, since it will grow > eventually if bytes passed in don't fit. > > I don't think it should be 32k. QUIC upload buffer : 14520, SPDY = 2852, and > Normal: 16384. 32k is larger than all of them. 16k sgtm. My concern is the need for reallocs/copies if data size exceeds buffer size. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:130: return mBuffer.position(); On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > hrm, this will be 0 if rewind() is called. > > getLength() is called in CronetUploadDataStream's constructor (before rewind() > is called), so it should not matter. I have tests > (CronetBuffferedOutputStream#testRewind and #testRewindWithoutContentLength) to > make sure it works. My concern here is fragility, few years from now somebody will call getLength() from another place and will be surprised by results. Can the limit() be used here instead? https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:138: int availableSpace = byteBuffer.capacity() - byteBuffer.position(); On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > Maybe rename availableSpace => readSize? > > It's actually not the read size, since the else clause writes the whole of > mBuffer, so I'd keep it as the availableSpace of the byteBuffer. ok https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:67: mBytesReceived++; On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > Rename mBytesReceived => mBytesWritten? > > Not sure about java code style guide, but Chromium C++ suggests prefix ++ like > > in ++mBytesReceived; > > Done. I believe the increment should be after the variable in Java. I haven't > seen the other variant that much. Looks like the same goes to the java files in > chromium: > https://code.google.com/p/chromium/codesearch#search/&q=file:java$%20%22%2B%2... sg https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: void checkReceivedEnoughContent() throws IOException { On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > not used? > > Used in CronetHttpURLConnection's startRequest() to make sure we have received > enough content. Ah, I see, maybe add comment? Not sure. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:204: // Start the request now since all headers can be sent. On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:55, mef wrote: > > why is this true? > > Because the user sets fixedLengthStreamingMode, so we know we can set the > content length header. > For the case where fixedLengthStreamingMode is not set, the request headers > should only be sent after all content has been buffered into memory. Yes, but why couldn't user call setRequestProperty() AFTER calling getOutputStream()? In other words, why call to getOutputStream should start the request? BTW, have you seen this: http://docs.oracle.com/javase/7/docs/api/java/net/HttpURLConnection.html#setF... A HttpRetryException will be thrown when reading the response if authentication or redirection are required https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:497: ((CronetFixedModeOutputStream) mOutputStream).checkReceivedEnoughContent(); On 2015/04/06 18:09:29, xunjieli wrote: > On 2015/04/06 16:14:54, mef wrote: > > Maybe make checkReceivedEnoughContent a base class method as well? > > Only CronetFixedModeOutputStream needs this method. CronetBufferedOutputStream's > check is in setConnected. These two classes already have a base class > OutputStream. Java does not allow multiple inheritance. If we were to make a > base class for these two, that base class has to implement methods in > OutputStream. However these two classes shouldn't share the common write > methods, for example. But it can be an abstract class that extends OutputStream but doesn't override any of its methods, right?
Patchset #15 (id:360001) has been deleted
Thanks for the review! PTAL. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:67: mBuffer = ByteBuffer.allocate(2048); On 2015/04/06 18:37:18, mef wrote: > On 2015/04/06 18:09:29, xunjieli wrote: > > On 2015/04/06 16:14:54, mef wrote: > > > Define 2048 as constant? While there, should it be like 32k? > > > > Hmm.. 2048 isn't used anywhere else in this file, so I am not sure whether > that > > should a constant. The value does not matter a whole lot, since it will grow > > eventually if bytes passed in don't fit. > > > > I don't think it should be 32k. QUIC upload buffer : 14520, SPDY = 2852, and > > Normal: 16384. 32k is larger than all of them. > > 16k sgtm. My concern is the need for reallocs/copies if data size exceeds buffer > size. Done. I don't know which one is better. I guess we could try with 16384 first. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java:130: return mBuffer.position(); On 2015/04/06 18:37:18, mef wrote: > On 2015/04/06 18:09:29, xunjieli wrote: > > On 2015/04/06 16:14:54, mef wrote: > > > hrm, this will be 0 if rewind() is called. > > > > getLength() is called in CronetUploadDataStream's constructor (before rewind() > > is called), so it should not matter. I have tests > > (CronetBuffferedOutputStream#testRewind and #testRewindWithoutContentLength) > to > > make sure it works. > > My concern here is fragility, few years from now somebody will call getLength() > from another place and will be surprised by results. > > Can the limit() be used here instead? If we have a buffer of 2048 bytes, and we write 18 bytes, the position will be 18, but the limit is 2048, so we can't use limit(). I hear you. It is not the best design. I don't have a better design on top of my mind. I need to think about it. Added a TODO. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: void checkReceivedEnoughContent() throws IOException { On 2015/04/06 18:37:18, mef wrote: > On 2015/04/06 18:09:29, xunjieli wrote: > > On 2015/04/06 16:14:54, mef wrote: > > > not used? > > > > Used in CronetHttpURLConnection's startRequest() to make sure we have received > > enough content. > > Ah, I see, maybe add comment? Not sure. Done. Right, this should deserve a comment. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:204: // Start the request now since all headers can be sent. On 2015/04/06 18:37:18, mef wrote: > On 2015/04/06 18:09:29, xunjieli wrote: > > On 2015/04/06 16:14:55, mef wrote: > > > why is this true? > > > > Because the user sets fixedLengthStreamingMode, so we know we can set the > > content length header. > > For the case where fixedLengthStreamingMode is not set, the request headers > > should only be sent after all content has been buffered into memory. > > Yes, but why couldn't user call setRequestProperty() AFTER calling > getOutputStream()? In other words, why call to getOutputStream should start the > request? getOutputStream() should try to establish a connection to the server for writing according to its specs. However, in the case where setFixedLengthStreamingMode is not used, connection establishment is delayed. Personally I find the intricacies of this API is a little confusing. http://stackoverflow.com/questions/10116961/can-you-explain-the-httpurlconnec... helps to understand. > > BTW, have you seen this: > > http://docs.oracle.com/javase/7/docs/api/java/net/HttpURLConnection.html#setF... > > A HttpRetryException will be thrown when reading the response if authentication > or redirection are required Done. Thanks for bringing this up. Changed the error in rewind below to match this, also updated test. https://codereview.chromium.org/966743003/diff/320001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:497: ((CronetFixedModeOutputStream) mOutputStream).checkReceivedEnoughContent(); On 2015/04/06 18:37:18, mef wrote: > On 2015/04/06 18:09:29, xunjieli wrote: > > On 2015/04/06 16:14:54, mef wrote: > > > Maybe make checkReceivedEnoughContent a base class method as well? > > > > Only CronetFixedModeOutputStream needs this method. > CronetBufferedOutputStream's > > check is in setConnected. These two classes already have a base class > > OutputStream. Java does not allow multiple inheritance. If we were to make a > > base class for these two, that base class has to implement methods in > > OutputStream. However these two classes shouldn't share the common write > > methods, for example. > > But it can be an abstract class that extends OutputStream but doesn't override > any of its methods, right? Done. Yes, you are right. Changed the code to do that, although checkReceivedEnoughContent is only applicable to one class and not the other, same goes to setConnected. I guess grouping them in the same place is cleaner nevertheless.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/966743003/#ps380001 (title: "Use an abstract class")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/966743003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Patchset #17 (id:420001) has been deleted
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/966743003/#ps440001 (title: "Remove unused")
Talked to Misha and Paul. I think we are going to CQ this, since there'll be time to iterate and I'd like to implement chunked support.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/966743003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/966743003/440001
Message was sent while issue was closed.
Committed patchset #17 (id:440001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/950576043d18caee3fc8bb28dfc432c3d6adcbda Cr-Commit-Position: refs/heads/master@{#325593} |
