Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2055083002:
[Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified (Closed)
|
Created:
4 years, 6 months ago by xunjieli Modified:
4 years, 6 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified
This CL fixes CronetFixedModeOutputStream to not send more
data than what has been written by consumer. Before this
patch, the code was trying to write the whole mBuffer even
if it is partially filled. This results in more data being
written to the CronetUploadDataStream.
This CL adds a test which will fail without this patch with
an error message that complains writing more bytes than
content-length.
BUG=618872
Committed: https://crrev.com/a5c5c291f250fc26e4eb6322edb776800c974e7f
Cr-Commit-Position: refs/heads/master@{#399706}
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : Paul's comments #
Total comments: 16
Patch Set 4 : Address Paul's comments #
Total comments: 33
Patch Set 5 : Address comments #
Total comments: 6
Patch Set 6 : address comments #Patch Set 7 : add @VisibleForTesting #
Total comments: 6
Patch Set 8 : address comments #
Messages
Total messages: 40 (17 generated)
Description was changed from ========== temp BUG= ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what is requested. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ==========
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what is requested. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ==========
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ==========
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message complaining that we are writing more than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code erroneously flipped mBuffer which can be partially written, which results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ==========
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ==========
https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (left): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:63: while (mBuffer.position() == mBuffer.limit()) { I can't remember why I put "while" here. I will think more about this tomorrow.
I'm pretty confused by the prior code and the new code. Neither has a comment describing what the position() and limit() of mBuffer really represent. The new code looks like mBuffer's position and limit mean different things when receiving from the client and when sending to the server, which makes it even harder for me to understand. I do like the fact that the new code avoids the compact() call which is a wasteful copy. What about moving to a model where writes into |mBuffer| (from the client) increment the limit(), and writes to the server increment the position()? https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/BUILD.gn:572: "test/javatests/src/org/chromium/net/QuicUploadTest.java", I think we need to add the corresponding GYP change, at least until the GYP files are removed from the tree. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (left): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:63: while (mBuffer.position() == mBuffer.limit()) { instead of "position == limit" we should say hasRemaining() https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:63: while (mBuffer.position() == mBuffer.limit()) { On 2016/06/10 00:10:48, xunjieli wrote: > I can't remember why I put "while" here. I will think more about this tomorrow. Actually I like "while" more than "if" because it implies that there will be space in the buffer after the loop. I think we should switch to "while" in both places. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:66: } We should move this loop into a separate function as it's copied in several places here, perhaps call it, ensureBufferHasRemaining(). https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:90: } This function seems like it could really use a short-circuit to avoid useless buffer copies. Something like: if (toSend >= sDefaultBufferLength && mBuffer.position() == 0) { // Send directly from |buffer|, perhaps by swapping it in for |mBuffer|. } https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:91: int sent = Math.min(toSend, mBuffer.limit() - mBuffer.position()); limit-position should be remaining()
> I'm pretty confused by the prior code and the new code. Neither has a comment > describing what the position() and limit() of mBuffer really represent. The new > code looks like mBuffer's position and limit mean different things when > receiving from the client and when sending to the server, which makes it even > harder for me to understand. I do like the fact that the new code avoids the > compact() call which is a wasteful copy. > What about moving to a model where writes into |mBuffer| (from the client) > increment the limit(), and writes to the server increment the position()? I think we do want to take advantage of ByteBuffer's put() methods. If we change the semantics of limit(), won't we not be able to take advantage of put() during writes? The old code is buggy, which I am not surprised considering I wrote that when I was a noogler (during which time, I thought that no-one would ever use this code). The new code is easier to understand. For writing, limit() is the index at which we should not write (available space to write is from position() to limit()). For reading, limit() is the index at we should not read (data to read is from position() to limit()). Before we read from mBuffer, we flip it to prepare for reading. Once we flip the mBuffer, the position() and limit() are correct for reading. WDYT? https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/BUILD.gn:572: "test/javatests/src/org/chromium/net/QuicUploadTest.java", On 2016/06/10 13:27:39, pauljensen wrote: > I think we need to add the corresponding GYP change, at least until the GYP > files are removed from the tree. GYP doesn't need java test files. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (left): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:63: while (mBuffer.position() == mBuffer.limit()) { On 2016/06/10 13:27:39, pauljensen wrote: > On 2016/06/10 00:10:48, xunjieli wrote: > > I can't remember why I put "while" here. I will think more about this > tomorrow. > > Actually I like "while" more than "if" because it implies that there will be > space in the buffer after the loop. I think we should switch to "while" in both > places. Done. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:66: } On 2016/06/10 13:27:39, pauljensen wrote: > We should move this loop into a separate function as it's copied in several > places here, perhaps call it, ensureBufferHasRemaining(). Done. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:90: } On 2016/06/10 13:27:39, pauljensen wrote: > This function seems like it could really use a short-circuit to avoid useless > buffer copies. Something like: > if (toSend >= sDefaultBufferLength && mBuffer.position() == 0) { > // Send directly from |buffer|, perhaps by swapping it in for |mBuffer|. > } Let's keep this as it is now. Swapping it for mBuffer also means that we just allocated one and decided to throw it away. We can't send directly from |buffer| unless UploadDataProvider can access it. It just seems that code will be complicated. Happy to review the CL if you want to give it a try :) https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:91: int sent = Math.min(toSend, mBuffer.limit() - mBuffer.position()); On 2016/06/10 13:27:39, pauljensen wrote: > limit-position should be remaining() Done.
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ==========
On 2016/06/10 14:05:00, xunjieli wrote: > > I'm pretty confused by the prior code and the new code. Neither has a comment > > describing what the position() and limit() of mBuffer really represent. The > new > > code looks like mBuffer's position and limit mean different things when > > receiving from the client and when sending to the server, which makes it even > > harder for me to understand. I do like the fact that the new code avoids the > > compact() call which is a wasteful copy. > > > What about moving to a model where writes into |mBuffer| (from the client) > > increment the limit(), and writes to the server increment the position()? > > I think we do want to take advantage of ByteBuffer's put() methods. If we change > the semantics of limit(), won't we not be able to take advantage of put() during > writes? The old code is buggy, which I am not surprised considering I wrote that > when I was a noogler (during which time, I thought that no-one would ever use > this code). > > The new code is easier to understand. For writing, limit() is the index at which > we should not write (available space to write is from position() to limit()). > For reading, limit() is the index at we should not read (data to read is from > position() to limit()). Before we read from mBuffer, we flip it to prepare for > reading. Once we flip the mBuffer, the position() and limit() are correct for > reading. > > WDYT? I'm not sure introducing complexity to allow us to use the put() method is worth it. The put() method is fairly inflexible, as you can see by the temporary adjustment of mBuffer.limit() in read() that was required. Anyhow, I'm fine with the way you've rewritten it now, but it definitly needs a big internal comment documenting how it works, including mentioning how it has to states, one where we're waiting on the client to fill our internal buffer, and one where we're waiting on the message loop to upload the data to the server from our internal buffer. Be sure to mention what mBuffer's position() and limit() represent in these two states, as it's very different. I am happy the rewrite removed the compact() call, this was an unnecessary runtime inefficiency. https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2055083002/diff/50001/components/cronet/andro... components/cronet/android/BUILD.gn:572: "test/javatests/src/org/chromium/net/QuicUploadTest.java", On 2016/06/10 14:05:00, xunjieli wrote: > On 2016/06/10 13:27:39, pauljensen wrote: > > I think we need to add the corresponding GYP change, at least until the GYP > > files are removed from the tree. > > GYP doesn't need java test files. Acknowledged. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:82: } is there an important benfit to this early return here? it looks like the rest of the function should work fine with a |count| of 0 and execute quickly. Removing this if statement would make this function more readable. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:95: mMessageLoop.loop(); can we combine these two lines from here and line line 67 and 104 into a function called "uploadBufferInternal()"? https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:96: } can we combine this if-statement with the one on line 66 into a function called "uploadIfComplete()"? https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:103: private void maybeWaitUntilBufferConsumed() { can we rename this to something a little easier to understand, like ensureBufferNotFull() or ensureBufferHasRemaining() https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: mBuffer.flip(); I don't think flip() should be in the loop as it has side effects that will break the loop I think. Perhaps we change this to: if (!mBuffer.hasRemaining()) { mBuffer.flip(); mMessageLoop.loop(); assert mBuffer.remaining() == mBuffer.capacity(); } https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:156: uploadDataSink.onReadSucceeded(false); any particular reason you moved this up? https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:163: mBuffer.limit(oldLimit); I was going to recommend changing this to the shorter: mBuffer.position(mBuffer.position() + byteBuffer.remaining()); byteBuffer.put(mBuffer.array(), mBuffer.position() - byteBuffer.remaining(), byteBuffer.remaining()); But that won't work with my idea to swap mBuffer with |buffer| in write() if |buffer| isn't array-backed. So I'll not mention it :)
Patchset #4 (id:90001) has been deleted
Thanks for the review! I added a comment in the code to document the two states that mBuffer can be in. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:82: } On 2016/06/10 15:05:25, pauljensen wrote: > is there an important benfit to this early return here? it looks like the rest > of the function should work fine with a |count| of 0 and execute quickly. > Removing this if statement would make this function more readable. Done. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:95: mMessageLoop.loop(); On 2016/06/10 15:05:25, pauljensen wrote: > can we combine these two lines from here and line line 67 and 104 into a > function called "uploadBufferInternal()"? Done. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:96: } On 2016/06/10 15:05:25, pauljensen wrote: > can we combine this if-statement with the one on line 66 into a function called > "uploadIfComplete()"? Done. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:103: private void maybeWaitUntilBufferConsumed() { On 2016/06/10 15:05:25, pauljensen wrote: > can we rename this to something a little easier to understand, like > ensureBufferNotFull() or ensureBufferHasRemaining() Done. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:105: mBuffer.flip(); On 2016/06/10 15:05:25, pauljensen wrote: > I don't think flip() should be in the loop as it has side effects that will > break the loop I think. Perhaps we change this to: > > if (!mBuffer.hasRemaining()) { > mBuffer.flip(); > mMessageLoop.loop(); > assert mBuffer.remaining() == mBuffer.capacity(); > } Done. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:156: uploadDataSink.onReadSucceeded(false); On 2016/06/10 15:05:25, pauljensen wrote: > any particular reason you moved this up? I think it might be better to notify the native stack first before we quit the message loop. This is how it was set up in CronetChunkedOutputStream.java. I am not sure if the previous approach has any re-entrancy issue. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:163: mBuffer.limit(oldLimit); On 2016/06/10 15:05:25, pauljensen wrote: > I was going to recommend changing this to the shorter: > > mBuffer.position(mBuffer.position() + byteBuffer.remaining()); > byteBuffer.put(mBuffer.array(), mBuffer.position() - byteBuffer.remaining(), > byteBuffer.remaining()); > > But that won't work with my idea to swap mBuffer with |buffer| in write() if > |buffer| isn't array-backed. So I'll not mention it :) Done.
https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:163: mBuffer.limit(oldLimit); On 2016/06/10 16:09:18, xunjieli wrote: > On 2016/06/10 15:05:25, pauljensen wrote: > > I was going to recommend changing this to the shorter: > > > > mBuffer.position(mBuffer.position() + byteBuffer.remaining()); > > byteBuffer.put(mBuffer.array(), mBuffer.position() - byteBuffer.remaining(), > > byteBuffer.remaining()); > > > > But that won't work with my idea to swap mBuffer with |buffer| in write() if > > |buffer| isn't array-backed. So I'll not mention it :) > > Done. Um I wasn't intending you to implement this. It's okay, I may revert back to the old way when I implement the idea to send directly from the client's buffer (which we can't assume to be array-backed and hence cannot call array() on). https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:33: // Internal buffer for storing the bytes written by consumer and for providing "written" has several possible meanings here (e.g. writing to the server, writing to mBuffer, writing to the UploadDataSink), as does consumer (e.g. Cronet API consumer, consumer in producer consumer model), how about "Internal buffer used to hold the bytes from the client"? https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:34: // bytes for the native stack to consume in UploadDataProvider.read(). the consuming isn't really done in UploadDataProvider.read(), how about "...from the client until they are copied to the UploadDataSink in UploadDataProvider.read()" https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, sDefaultBufferLength->sDefaultBufferLength bytes https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, write->provide https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, consumer->the client https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:36: // and wait for UploadDataProvider.read() to be called so mBuffer is cleared so->, after which point https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:37: // for next round of writing. During writing, the buffer's position points for next round of writing->so the client can fill it again. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:37: // for next round of writing. During writing, the buffer's position points During writing->While the client is filling the buffer (via {@code write()}) https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:38: // to the next byte to be written, and limit is the index at which no byte written->provided by the client https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:38: // to the next byte to be written, and limit is the index at which no byte "is the index..."->"points to the end of the buffer" https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:41: // points to the next byte to be read, and limit is the index at which no read->copied to the UploadDataSink https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:41: // points to the next byte to be read, and limit is the index at which no "is the index..."->"points to the end of the data available to be copied to the UploadDataSink" https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:42: // byte should be read. When the UploadDataProvider is done reading from "is done..."->"has provided all remaining bytes from the buffer to the UploadDataSink, it clears the buffer so the client can fill it again" https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. the->for the https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. since...->because the client has provided all bytes to be uploaded and there is no need to wait for or expect the client to provide more bytes. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. data->mBuffer's contents https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:119: * Helper function to upload the buffer to the native stack. This function buffer->{@code mBuffer}
Thanks for the review! PTAL. https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/70001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:163: mBuffer.limit(oldLimit); On 2016/06/10 17:45:44, pauljensen wrote: > On 2016/06/10 16:09:18, xunjieli wrote: > > On 2016/06/10 15:05:25, pauljensen wrote: > > > I was going to recommend changing this to the shorter: > > > > > > mBuffer.position(mBuffer.position() + byteBuffer.remaining()); > > > byteBuffer.put(mBuffer.array(), mBuffer.position() - > byteBuffer.remaining(), > > > byteBuffer.remaining()); > > > > > > But that won't work with my idea to swap mBuffer with |buffer| in write() if > > > |buffer| isn't array-backed. So I'll not mention it :) > > > > Done. > > Um I wasn't intending you to implement this. It's okay, I may revert back to > the old way when I implement the idea to send directly from the client's buffer > (which we can't assume to be array-backed and hence cannot call array() on). Ah, I see. I was confused by "it". Done. I have reverted to the original. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java (right): https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:33: // Internal buffer for storing the bytes written by consumer and for providing On 2016/06/10 17:45:44, pauljensen wrote: > "written" has several possible meanings here (e.g. writing to the server, > writing to mBuffer, writing to the UploadDataSink), as does consumer (e.g. > Cronet API consumer, consumer in producer consumer model), how about "Internal > buffer used to hold the bytes from the client"? Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:34: // bytes for the native stack to consume in UploadDataProvider.read(). On 2016/06/10 17:45:44, pauljensen wrote: > the consuming isn't really done in UploadDataProvider.read(), how about "...from > the client until they are copied to the UploadDataSink in > UploadDataProvider.read()" Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, On 2016/06/10 17:45:45, pauljensen wrote: > sDefaultBufferLength->sDefaultBufferLength bytes Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, On 2016/06/10 17:45:44, pauljensen wrote: > consumer->the client Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:35: // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength, On 2016/06/10 17:45:45, pauljensen wrote: > sDefaultBufferLength->sDefaultBufferLength bytes Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:36: // and wait for UploadDataProvider.read() to be called so mBuffer is cleared On 2016/06/10 17:45:44, pauljensen wrote: > so->, after which point Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:37: // for next round of writing. During writing, the buffer's position points On 2016/06/10 17:45:44, pauljensen wrote: > for next round of writing->so the client can fill it again. Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:37: // for next round of writing. During writing, the buffer's position points On 2016/06/10 17:45:44, pauljensen wrote: > During writing->While the client is filling the buffer (via {@code write()}) Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:38: // to the next byte to be written, and limit is the index at which no byte On 2016/06/10 17:45:44, pauljensen wrote: > "is the index..."->"points to the end of the buffer" Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:41: // points to the next byte to be read, and limit is the index at which no On 2016/06/10 17:45:44, pauljensen wrote: > "is the index..."->"points to the end of the data available to be copied to the > UploadDataSink" Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:41: // points to the next byte to be read, and limit is the index at which no On 2016/06/10 17:45:44, pauljensen wrote: > read->copied to the UploadDataSink Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:42: // byte should be read. When the UploadDataProvider is done reading from On 2016/06/10 17:45:44, pauljensen wrote: > "is done..."->"has provided all remaining bytes from the buffer to the > UploadDataSink, it clears the buffer so the client can fill it again" Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. On 2016/06/10 17:45:44, pauljensen wrote: > data->mBuffer's contents Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. On 2016/06/10 17:45:44, pauljensen wrote: > the->for the Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:108: * Waits the native stack to upload data since all bytes have been written. On 2016/06/10 17:45:44, pauljensen wrote: > the->for the Done. https://codereview.chromium.org/2055083002/diff/110001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java:119: * Helper function to upload the buffer to the native stack. This function On 2016/06/10 17:45:44, pauljensen wrote: > buffer->{@code mBuffer} Done.
https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java (right): https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:5: package org.chromium.net; I think this should be moved to the urlconnection package and directory https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:33: JSONObject quicParams = new JSONObject().put("host_whitelist", "test.example.com"); use QuicTestServer.getServerHost() https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:60: } use Arrays.fill(largeData, 97)
Patchset #6 (id:150001) has been deleted
Thanks for the review! I wasn't able to add @VisibleForTesting to CronetEngine.java. Looks like it is because Cronet api jar isn't compiled with org.chromium.base. PTAL. Thanks https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java (right): https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:5: package org.chromium.net; On 2016/06/13 12:22:13, pauljensen wrote: > I think this should be moved to the urlconnection package and directory Done. https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:33: JSONObject quicParams = new JSONObject().put("host_whitelist", "test.example.com"); On 2016/06/13 12:22:13, pauljensen wrote: > use QuicTestServer.getServerHost() Done. https://codereview.chromium.org/2055083002/diff/130001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicUploadTest.java:60: } On 2016/06/13 12:22:13, pauljensen wrote: > use Arrays.fill(largeData, 97) Done.
On 2016/06/13 14:23:24, xunjieli wrote: > Thanks for the review! I wasn't able to add @VisibleForTesting to > CronetEngine.java. Looks like it is because Cronet api jar isn't compiled with > org.chromium.base. PTAL. Thanks @VisibleForTesting isn't part of org.chromium.base, it's part of android.support.annotation. It should be pulled in for the API code as cronet_api target depends on //third_party/android_tools:android_support_annotations_javalib
On 2016/06/13 15:42:06, pauljensen wrote: > On 2016/06/13 14:23:24, xunjieli wrote: > > Thanks for the review! I wasn't able to add @VisibleForTesting to > > CronetEngine.java. Looks like it is because Cronet api jar isn't compiled with > > org.chromium.base. PTAL. Thanks > > @VisibleForTesting isn't part of org.chromium.base, it's part of > android.support.annotation. It should be pulled in for the API code as > cronet_api target depends on > //third_party/android_tools:android_support_annotations_javalib Done. I see. I didn't realize that there is another @VisibleForTesting different from the one in org.chromium.base.VisibleForTesting https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/.... PTAL. Thanks
Paul: a friendly ping.. If possible, I am hoping to get this in this week's dev build, which is usually cut on Tuesday.
https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:593: * @return the builder to facilitate chaining. needs @hide https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:282: CronetFixedModeOutputStream.setDefaultBufferLengthForTesting(18384); why the buffer size change? https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:285: testOneMassiveWrite(); can you add a comment about what this call does? I assume it tests a large upload. how does this compile? I don't see an import of this function.
Thanks for the review! PTAL https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:593: * @return the builder to facilitate chaining. On 2016/06/14 12:50:31, pauljensen wrote: > needs @hide Done. https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java (right): https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:282: CronetFixedModeOutputStream.setDefaultBufferLengthForTesting(18384); On 2016/06/14 12:50:31, pauljensen wrote: > why the buffer size change? That's a very good question. (I should have commented on this). The size is changed so this test can catch the regression. The regression happens when the last mBuffer has more bytes than the what the native buffer size can hold. In this case, the last mBuffer will contain 1,800,000 % 18384 = 16752 > 16384 (native buffer size). The previous buffer size wasn't fully exercising the code. https://codereview.chromium.org/2055083002/diff/190001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java:285: testOneMassiveWrite(); On 2016/06/14 12:50:31, pauljensen wrote: > can you add a comment about what this call does? I assume it tests a large > upload. > how does this compile? I don't see an import of this function. It's a test method. I've added a comment.
lgtm
thanks a lot for the review!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2055083002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2055083002/210001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:210001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 ========== to ========== [Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified This CL fixes CronetFixedModeOutputStream to not send more data than what has been written by consumer. Before this patch, the code was trying to write the whole mBuffer even if it is partially filled. This results in more data being written to the CronetUploadDataStream. This CL adds a test which will fail without this patch with an error message that complains writing more bytes than content-length. BUG=618872 Committed: https://crrev.com/a5c5c291f250fc26e4eb6322edb776800c974e7f Cr-Commit-Position: refs/heads/master@{#399706} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a5c5c291f250fc26e4eb6322edb776800c974e7f Cr-Commit-Position: refs/heads/master@{#399706} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
