Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030353002:
UploadDataStream now returns errors on failures (Closed)
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 6 months ago Reviewers:
mmenke 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. |
DescriptionMake UploadElementsDataStream return errors on failures.
It previously zero-padded the response on errors, which had
the effect of uploading corrupted data, without indicating
there was a problem to consumers.
Three earlier CLs upload consumers of this class to support
this new behavior:
HttpStreamParser
https://codereview.chromium.org/2007013003/
SpdyHttpStream:
https://codereview.chromium.org/2022053002/
QuicHttpStream
https://codereview.chromium.org/2035643002/
And main change in ElementsUploadDataStream
https://codereview.chromium.org/2030353002/ <------ current
BUG=517615
Committed: https://crrev.com/e869bf5e1c460baf8a96f5b00407a76dad7e7ef8
Cr-Commit-Position: refs/heads/master@{#401636}
Patch Set 1 #Patch Set 2 : comments in the header #
Total comments: 8
Patch Set 3 : Matt's comments, rebasing + fixed unittests #
Total comments: 10
Patch Set 4 : fixed issues and combined with HttpStreamParser cl due to dependencies #
Total comments: 1
Patch Set 5 : removing net::OK check #Patch Set 6 : rebased #
Messages
Total messages: 67 (32 generated)
Description was changed from ========== failing read BUG= ========== to ========== Introduce error return on read failure in UploadDataStream. UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
Description was changed from ========== Introduce error return on read failure in UploadDataStream. UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Introduce returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Description was changed from ========== Introduce returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Description was changed from ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Description was changed from ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. The UploadDataStream API is changed so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Description was changed from ========== Returning errors on a read failure in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. The UploadDataStream API is changed so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Returning errors on read failures in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. The UploadDataStream API is changed so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Description was changed from ========== Returning errors on read failures in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting returning corrupted data to callers. The UploadDataStream API is changed so that Read will return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Returning errors on read failures in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Done, please take a look
On 2016/06/03 12:01:01, maksims wrote: > Done, please take a look not ptal. forgotten about the tests. Btw, HttpNetworkTransactionTest, UploadFileSmallerThanLength relies on HttpStreamParser, but it's not upstreamed and I cannot make the unittest working with the old implementation that is in the trunk.
On 2016/06/03 13:03:36, maksims wrote: > On 2016/06/03 12:01:01, maksims wrote: > > Done, please take a look > > not ptal. forgotten about the tests. > > Btw, HttpNetworkTransactionTest, UploadFileSmallerThanLength relies on > HttpStreamParser, but it's not upstreamed and I cannot make the unittest working > with the old implementation that is in the trunk. You can make git branches on top of other branches, so you could create, for instance, your HttpStreamParser branch of top of master, The SPDY one on top of that, the QUIC on top of that, and this on top of both of that. Branch management can get a bit complicated if you opt for that route, though. The try system is smart enough that when you run a change on the trybots, it will apply all the intermediate patches as well.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
On 2016/06/03 13:09:21, mmenke wrote: > On 2016/06/03 13:03:36, maksims wrote: > > On 2016/06/03 12:01:01, maksims wrote: > > > Done, please take a look > > > > not ptal. forgotten about the tests. > > > > Btw, HttpNetworkTransactionTest, UploadFileSmallerThanLength relies on > > HttpStreamParser, but it's not upstreamed and I cannot make the unittest > working > > with the old implementation that is in the trunk. > > You can make git branches on top of other branches, so you could create, for > instance, your HttpStreamParser branch of top of master, The SPDY one on top of > that, the QUIC on top of that, and this on top of both of that. > > Branch management can get a bit complicated if you opt for that route, though. > The try system is smart enough that when you run a change on the trybots, it > will apply all the intermediate patches as well. It seems like Spdy/SpdyNetworkTransactionTest.ResponseBeforePostCompletes hangs on helper.WaitForHeaders();. Seems like you said in Spdy review the callback is NULLed and not called. Tests are here - https://codereview.chromium.org/2037353002/
Patchset #3 (id:120001) has been deleted
Patchset #4 (id:60002) has been deleted
Patchset #3 (id:140001) has been deleted
https://codereview.chromium.org/2030353002/diff/20001/net/base/elements_uploa... File net/base/elements_upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/elements_uploa... net/base/elements_upload_data_stream.cc:153: read_failed_ = true; Instead of just setting read_failed_ to true, can we replace read_failed_ with an integer (That's net::OK by default), and set it to the error code stored in result here, and then return that error code in ElementsUploadDataStream::ReadElements? https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.cc:113: DCHECK(initialized_successfully_); I think this DCHECK can go before the if. https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.cc:122: DCHECK(result > 0 || is_eof_); This DCHECK no longer does anything. Suggest moving it above the if, and replacing it with: DCHECK(result != 0 || is_eof_); DCHECK_NE(ERR_IO_PENDING, result); https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... File net/base/upload_data_stream.h (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.h:61: // with an error value. Think this should separate ERR_FAILED with ERR_IO_PENDING (They can also return ERR_IO_PENDING on success). Maybe just remove this paragraph, and modify the first one to mention the failure case. i.e., something like: When possible, reads up to |buf_len| bytes synchronously from the upload data stream to |buf| and returns the number of bytes read. Returns a network error on synchronous failure. Returns ERR_IO_PENDING and calls |callback| with the number of bytes read or error code on asynchronous completion. Partial reads are allowed....
Oh, and I plan to take a look at the tests, too, but may not get to them today (Since we're going to be waiting on Bence to get back in town, de-prioritizing this a bit - certainly intend to get to it in the next day or so).
On 2016/06/06 16:23:41, mmenke wrote: > Oh, and I plan to take a look at the tests, too, but may not get to them today > (Since we're going to be waiting on Bence to get back in town, de-prioritizing > this a bit - certainly intend to get to it in the next day or so). Ok, no hurry on this.
On 2016/06/06 16:49:54, maksims wrote: > On 2016/06/06 16:23:41, mmenke wrote: > > Oh, and I plan to take a look at the tests, too, but may not get to them today > > (Since we're going to be waiting on Bence to get back in town, de-prioritizing > > this a bit - certainly intend to get to it in the next day or so). > > Ok, no hurry on this. Sorry, forgot about this one. I'll try to get to it this afternoon.
On 2016/06/15 16:05:15, mmenke wrote: > On 2016/06/06 16:49:54, maksims wrote: > > On 2016/06/06 16:23:41, mmenke wrote: > > > Oh, and I plan to take a look at the tests, too, but may not get to them > today > > > (Since we're going to be waiting on Bence to get back in town, > de-prioritizing > > > this a bit - certainly intend to get to it in the next day or so). > > > > Ok, no hurry on this. > > Sorry, forgot about this one. I'll try to get to it this afternoon. Actually, I have already solved the problematic test cases with this one. Waiting for the SPDY to be approved.
Patchset #3 (id:170001) has been deleted
Patchset #5 (id:230001) has been deleted
Patchset #4 (id:210001) has been deleted
Patchset #3 (id:190001) has been deleted
ptal. Tests are here https://codereview.chromium.org/2077353002/ https://codereview.chromium.org/2030353002/diff/20001/net/base/elements_uploa... File net/base/elements_upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/elements_uploa... net/base/elements_upload_data_stream.cc:153: read_failed_ = true; On 2016/06/06 16:22:33, mmenke wrote: > Instead of just setting read_failed_ to true, can we replace read_failed_ with > an integer (That's net::OK by default), and set it to the error code stored in > result here, and then return that error code in > ElementsUploadDataStream::ReadElements? Done. https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... File net/base/upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.cc:113: DCHECK(initialized_successfully_); On 2016/06/06 16:22:33, mmenke wrote: > I think this DCHECK can go before the if. Done. https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.cc:122: DCHECK(result > 0 || is_eof_); On 2016/06/06 16:22:33, mmenke wrote: > This DCHECK no longer does anything. > > Suggest moving it above the if, and replacing it with: > > DCHECK(result != 0 || is_eof_); > DCHECK_NE(ERR_IO_PENDING, result); Done. https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... File net/base/upload_data_stream.h (right): https://codereview.chromium.org/2030353002/diff/20001/net/base/upload_data_st... net/base/upload_data_stream.h:61: // with an error value. On 2016/06/06 16:22:33, mmenke wrote: > Think this should separate ERR_FAILED with ERR_IO_PENDING (They can also return > ERR_IO_PENDING on success). Maybe just remove this paragraph, and modify the > first one to mention the failure case. i.e., something like: > > > When possible, reads up to |buf_len| bytes synchronously from the upload > data stream to |buf| and returns the number of bytes read. Returns a network > error on synchronous failure. Returns ERR_IO_PENDING and calls |callback| > with the number of bytes read or error code on asynchronous completion. > Partial reads are allowed.... Should I remove your TODO now?
By the way, how will I commit the patch? Should I create a separate CL with all the changes together in order to commit that? Of course after you approve this one.
On 2016/06/21 11:10:24, maksims wrote: > By the way, how will I commit the patch? Should I create a separate CL with all > the changes together in order to commit that? Of course after you approve this > one. I'd say land them one at a time, as long as each CL comes with its own tests. This one should land last, since it depends on all the others. I'd suggest starting with the HttpStreamParser CL as the most basic, then SPDY and QUIC (Order doesn't really matter), and finally this one. I plan to take to take a look at this later today..
On 2016/06/21 15:29:37, mmenke wrote: > On 2016/06/21 11:10:24, maksims wrote: > > By the way, how will I commit the patch? Should I create a separate CL with > all > > the changes together in order to commit that? Of course after you approve this > > one. > > I'd say land them one at a time, as long as each CL comes with its own tests. > This one should land last, since it depends on all the others. I'd suggest > starting with the HttpStreamParser CL as the most basic, then SPDY and QUIC > (Order doesn't really matter), and finally this one. > > I plan to take to take a look at this later today.. Oh, and you'll probably need to land the upload_data_stream.h / .cc changes in the set with HttpStreamParser, so your test classes fail as they should.
This looks good! I'll sign off on this once the others are landed. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... File net/base/elements_upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.cc:106: while (!read_failed_ && element_index_ < element_readers_.size()) { !read_failed_ should be read_failed_ == OK, for clarity. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.cc:128: if (read_failed_ != OK) I'd suggest this instead: if (buf->BytesConsumed() > 0) return buf->BytesConsumed(); return read_failed_; That way, if we read bytes and have an error, we return the read bytes first. Probably doesn't matter, but seems safer. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... File net/base/elements_upload_data_stream.h (right): https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.h:80: // True if an error occurred during read operation. This comment needs to be updated. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.h:81: int read_failed_; read_error_, maybe? https://codereview.chromium.org/2030353002/diff/250001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2030353002/diff/250001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:9692: EXPECT_FALSE(response->headers); Should remove code below this line (Code shouldn't be calling ReadTransaction after a failure. The fact that it works isn't great, but is probably true for other errors, too)
Description was changed from ========== Returning errors on read failures in UploadDataStream UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== HttpStreamParser now handles errors returned by UploadDataStream::Read() UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream. Has to be combined with HttpStreamParser cl due to dependent changes that cannot be landed separately. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Patchset #5 (id:290001) has been deleted
Patchset #4 (id:270001) has been deleted
Patchset #5 (id:330001) has been deleted
Patchset #4 (id:310001) has been deleted
Patchset #4 (id:350001) has been deleted
Description was changed from ========== HttpStreamParser now handles errors returned by UploadDataStream::Read() UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream. Has to be combined with HttpStreamParser cl due to dependent changes that cannot be landed separately. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== HttpStreamParser now handles errors returned by UploadDataStream::Read() UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream. Note: this has to be combined with HttpStreamParser cl due to dependent changes that cannot be landed separately. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... File net/base/elements_upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.cc:106: while (!read_failed_ && element_index_ < element_readers_.size()) { On 2016/06/21 21:13:13, mmenke wrote: > !read_failed_ should be read_failed_ == OK, for clarity. Done. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.cc:128: if (read_failed_ != OK) On 2016/06/21 21:13:13, mmenke wrote: > I'd suggest this instead: > > if (buf->BytesConsumed() > 0) > return buf->BytesConsumed(); > > return read_failed_; > > That way, if we read bytes and have an error, we return the read bytes first. > Probably doesn't matter, but seems safer. Done. It sitll has to check if there is now error, because it might fail on another read round. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... File net/base/elements_upload_data_stream.h (right): https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.h:80: // True if an error occurred during read operation. On 2016/06/21 21:13:14, mmenke wrote: > This comment needs to be updated. Done. https://codereview.chromium.org/2030353002/diff/250001/net/base/elements_uplo... net/base/elements_upload_data_stream.h:81: int read_failed_; On 2016/06/21 21:13:14, mmenke wrote: > read_error_, maybe? Done. https://codereview.chromium.org/2030353002/diff/250001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2030353002/diff/250001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:9692: EXPECT_FALSE(response->headers); On 2016/06/21 21:13:14, mmenke wrote: > Should remove code below this line (Code shouldn't be calling ReadTransaction > after a failure. The fact that it works isn't great, but is probably true for > other errors, too) Done.
https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... File net/base/elements_upload_data_stream.cc (right): https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > 0 && read_error_ == net::OK) I don't think you need the read error check here - if we read 5 bytes and then run into an error, just checking bytes consumed here means we're return 5 on the first read...When ReadElements is called again, we'd skip the while loop (Since read_error_ is still not OK), skip this if (Since BytesConsumed is 0), and then return the error....right?
On 2016/06/22 18:10:20, mmenke wrote: > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > File net/base/elements_upload_data_stream.cc (right): > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > 0 && > read_error_ == net::OK) > I don't think you need the read error check here - if we read 5 bytes and then > run into an error, just checking bytes consumed here means we're return 5 on the > first read...When ReadElements is called again, we'd skip the while loop (Since > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), and then > return the error....right? No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will return BytesConsumed instead of the error.
On 2016/06/22 18:41:28, maksims wrote: > On 2016/06/22 18:10:20, mmenke wrote: > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > File net/base/elements_upload_data_stream.cc (right): > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > 0 && > > read_error_ == net::OK) > > I don't think you need the read error check here - if we read 5 bytes and then > > run into an error, just checking bytes consumed here means we're return 5 on > the > > first read...When ReadElements is called again, we'd skip the while loop > (Since > > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), and > then > > return the error....right? > > No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will return > BytesConsumed instead of the error. It will return*, not we
On 2016/06/22 18:42:05, maksims wrote: > On 2016/06/22 18:41:28, maksims wrote: > > On 2016/06/22 18:10:20, mmenke wrote: > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > File net/base/elements_upload_data_stream.cc (right): > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > 0 && > > > read_error_ == net::OK) > > > I don't think you need the read error check here - if we read 5 bytes and > then > > > run into an error, just checking bytes consumed here means we're return 5 on > > the > > > first read...When ReadElements is called again, we'd skip the while loop > > (Since > > > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), and > > then > > > return the error....right? > > > > No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will return > > BytesConsumed instead of the error. > > It will return*, not we But it's created again in each ElementsUploadDataStream::ReadInternal call, so it would restart from 0, no?
On 2016/06/22 18:46:32, mmenke wrote: > On 2016/06/22 18:42:05, maksims wrote: > > On 2016/06/22 18:41:28, maksims wrote: > > > On 2016/06/22 18:10:20, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > File net/base/elements_upload_data_stream.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > 0 > && > > > > read_error_ == net::OK) > > > > I don't think you need the read error check here - if we read 5 bytes and > > then > > > > run into an error, just checking bytes consumed here means we're return 5 > on > > > the > > > > first read...When ReadElements is called again, we'd skip the while loop > > > (Since > > > > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), and > > > then > > > > return the error....right? > > > > > > No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will > return > > > BytesConsumed instead of the error. > > > > It will return*, not we > > But it's created again in each ElementsUploadDataStream::ReadInternal call, so > it would restart from 0, no? But ReadInternal is called ones and then there is a "loop" of OnReadElementCompleted->ReadElements->OnReadElementCompleted. I am saying so because ElementsUploadDataStreamTest.FileSmallerThanLength test fails if there is no "read_error_ == net::OK" test. Callback just returns 10 (first read bytes) instead of actual error.
On 2016/06/22 18:56:55, maksims wrote: > On 2016/06/22 18:46:32, mmenke wrote: > > On 2016/06/22 18:42:05, maksims wrote: > > > On 2016/06/22 18:41:28, maksims wrote: > > > > On 2016/06/22 18:10:20, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > > File net/base/elements_upload_data_stream.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > > 0 > > && > > > > > read_error_ == net::OK) > > > > > I don't think you need the read error check here - if we read 5 bytes > and > > > then > > > > > run into an error, just checking bytes consumed here means we're return > 5 > > on > > > > the > > > > > first read...When ReadElements is called again, we'd skip the while loop > > > > (Since > > > > > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), > and > > > > then > > > > > return the error....right? > > > > > > > > No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will > > return > > > > BytesConsumed instead of the error. > > > > > > It will return*, not we > > > > But it's created again in each ElementsUploadDataStream::ReadInternal call, so > > it would restart from 0, no? > > But ReadInternal is called ones and then there is a "loop" of > OnReadElementCompleted->ReadElements->OnReadElementCompleted. > > I am saying so because ElementsUploadDataStreamTest.FileSmallerThanLength test > fails if there is no "read_error_ == net::OK" test. > Callback just returns 10 (first read bytes) instead of actual error. I guess that was the reason why there was "if (read_failed_)" before "return buf->BytesConsumed()"
On 2016/06/22 18:56:55, maksims wrote: > On 2016/06/22 18:46:32, mmenke wrote: > > On 2016/06/22 18:42:05, maksims wrote: > > > On 2016/06/22 18:41:28, maksims wrote: > > > > On 2016/06/22 18:10:20, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > > File net/base/elements_upload_data_stream.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2030353002/diff/370001/net/base/elements_uplo... > > > > > net/base/elements_upload_data_stream.cc:128: if (buf->BytesConsumed() > > 0 > > && > > > > > read_error_ == net::OK) > > > > > I don't think you need the read error check here - if we read 5 bytes > and > > > then > > > > > run into an error, just checking bytes consumed here means we're return > 5 > > on > > > > the > > > > > first read...When ReadElements is called again, we'd skip the while loop > > > > (Since > > > > > read_error_ is still not OK), skip this if (Since BytesConsumed is 0), > and > > > > then > > > > > return the error....right? > > > > > > > > No, BytesConsumed will be 5. It is a sum of total bytes. Thus, we will > > return > > > > BytesConsumed instead of the error. > > > > > > It will return*, not we > > > > But it's created again in each ElementsUploadDataStream::ReadInternal call, so > > it would restart from 0, no? > > But ReadInternal is called ones and then there is a "loop" of > OnReadElementCompleted->ReadElements->OnReadElementCompleted. > > I am saying so because ElementsUploadDataStreamTest.FileSmallerThanLength test > fails if there is no "read_error_ == net::OK" test. > Callback just returns 10 (first read bytes) instead of actual error. That seems reasonable...The next read would then return an error, right?
Yes, you are right. New patch without the check is uploaded.
On 2016/06/22 19:12:20, maksims wrote: > Yes, you are right. New patch without the check is uploaded. Looks good! Think it's about time to start landing these, following the order I suggested earlier.
On 2016/06/22 19:26:56, mmenke wrote: > On 2016/06/22 19:12:20, maksims wrote: > > Yes, you are right. New patch without the check is uploaded. > > Looks good! Think it's about time to start landing these, following the order I > suggested earlier. This one should land first, otherwise Http, Spdy and Quic just fail. Their unittests are dependent on UploadDataStream::OnReadCompleted(), which should pass through the errors.
On 2016/06/22 20:00:40, maksims wrote: > On 2016/06/22 19:26:56, mmenke wrote: > > On 2016/06/22 19:12:20, maksims wrote: > > > Yes, you are right. New patch without the check is uploaded. > > > > Looks good! Think it's about time to start landing these, following the order > I > > suggested earlier. > > This one should land first, otherwise Http, Spdy and Quic just fail. Their > unittests are dependent on UploadDataStream::OnReadCompleted(), which should > pass through the errors. That is why I added http stream parser changes into this cl.
On 2016/06/22 20:02:40, maksims wrote: > On 2016/06/22 20:00:40, maksims wrote: > > On 2016/06/22 19:26:56, mmenke wrote: > > > On 2016/06/22 19:12:20, maksims wrote: > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > Looks good! Think it's about time to start landing these, following the > order > > I > > > suggested earlier. > > > > This one should land first, otherwise Http, Spdy and Quic just fail. Their > > unittests are dependent on UploadDataStream::OnReadCompleted(), which should > > pass through the errors. > > That is why I added http stream parser changes into this cl. Unfortunately, this one can't land first - we need to keep Chrome continuously functional, since we ship it out nightly, and this breaks that. If we move the upload_data_stream.h / upload_data_stream.cc changes into the HttpStreamParser CL first, that's all that's needed to get the other CLs working, isn't it?
On 2016/06/22 20:24:07, mmenke wrote: > On 2016/06/22 20:02:40, maksims wrote: > > On 2016/06/22 20:00:40, maksims wrote: > > > On 2016/06/22 19:26:56, mmenke wrote: > > > > On 2016/06/22 19:12:20, maksims wrote: > > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > > > Looks good! Think it's about time to start landing these, following the > > order > > > I > > > > suggested earlier. > > > > > > This one should land first, otherwise Http, Spdy and Quic just fail. Their > > > unittests are dependent on UploadDataStream::OnReadCompleted(), which should > > > pass through the errors. > > > > That is why I added http stream parser changes into this cl. > > Unfortunately, this one can't land first - we need to keep Chrome continuously > functional, since we ship it out nightly, and this breaks that. If we move the > upload_data_stream.h / upload_data_stream.cc changes into the HttpStreamParser > CL first, that's all that's needed to get the other CLs working, isn't it? Now that the other three have landed, LGTM! Don't forgot to merge with the other landed cls (Or just remove the changes to everything but the upload_elements_* files)
Description was changed from ========== HttpStreamParser now handles errors returned by UploadDataStream::Read() UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream. Note: this has to be combined with HttpStreamParser cl due to dependent changes that cannot be landed separately. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== UploadDataStream now returns errors on failures UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
On 2016/06/23 15:07:18, mmenke wrote: > On 2016/06/22 20:24:07, mmenke wrote: > > On 2016/06/22 20:02:40, maksims wrote: > > > On 2016/06/22 20:00:40, maksims wrote: > > > > On 2016/06/22 19:26:56, mmenke wrote: > > > > > On 2016/06/22 19:12:20, maksims wrote: > > > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > > > > > Looks good! Think it's about time to start landing these, following the > > > order > > > > I > > > > > suggested earlier. > > > > > > > > This one should land first, otherwise Http, Spdy and Quic just fail. Their > > > > unittests are dependent on UploadDataStream::OnReadCompleted(), which > should > > > > pass through the errors. > > > > > > That is why I added http stream parser changes into this cl. > > > > Unfortunately, this one can't land first - we need to keep Chrome continuously > > functional, since we ship it out nightly, and this breaks that. If we move > the > > upload_data_stream.h / upload_data_stream.cc changes into the HttpStreamParser > > CL first, that's all that's needed to get the other CLs working, isn't it? > > Now that the other three have landed, LGTM! Don't forgot to merge with the > other landed cls (Or just remove the changes to everything but the > upload_elements_* files) How about the description?
On 2016/06/23 16:02:49, maksims wrote: > On 2016/06/23 15:07:18, mmenke wrote: > > On 2016/06/22 20:24:07, mmenke wrote: > > > On 2016/06/22 20:02:40, maksims wrote: > > > > On 2016/06/22 20:00:40, maksims wrote: > > > > > On 2016/06/22 19:26:56, mmenke wrote: > > > > > > On 2016/06/22 19:12:20, maksims wrote: > > > > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > > > > > > > Looks good! Think it's about time to start landing these, following > the > > > > order > > > > > I > > > > > > suggested earlier. > > > > > > > > > > This one should land first, otherwise Http, Spdy and Quic just fail. > Their > > > > > unittests are dependent on UploadDataStream::OnReadCompleted(), which > > should > > > > > pass through the errors. > > > > > > > > That is why I added http stream parser changes into this cl. > > > > > > Unfortunately, this one can't land first - we need to keep Chrome > continuously > > > functional, since we ship it out nightly, and this breaks that. If we move > > the > > > upload_data_stream.h / upload_data_stream.cc changes into the > HttpStreamParser > > > CL first, that's all that's needed to get the other CLs working, isn't it? > > > > Now that the other three have landed, LGTM! Don't forgot to merge with the > > other landed cls (Or just remove the changes to everything but the > > upload_elements_* files) > > How about the description? Good point. Maybe something like: Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/
Description was changed from ========== UploadDataStream now returns errors on failures UploadDataStream was written with the assumption UploadDataStream::Read() cannot fail, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero padded the response, resulting returning corrupted data to callers. The UploadDataStream API is changed now to make Read return errors on failures. This cl introduces error returns on a read failure in UploadDataStream There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in ElementsUploadDataStream https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
On 2016/06/23 16:05:37, mmenke wrote: > On 2016/06/23 16:02:49, maksims wrote: > > On 2016/06/23 15:07:18, mmenke wrote: > > > On 2016/06/22 20:24:07, mmenke wrote: > > > > On 2016/06/22 20:02:40, maksims wrote: > > > > > On 2016/06/22 20:00:40, maksims wrote: > > > > > > On 2016/06/22 19:26:56, mmenke wrote: > > > > > > > On 2016/06/22 19:12:20, maksims wrote: > > > > > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > > > > > > > > > Looks good! Think it's about time to start landing these, following > > the > > > > > order > > > > > > I > > > > > > > suggested earlier. > > > > > > > > > > > > This one should land first, otherwise Http, Spdy and Quic just fail. > > Their > > > > > > unittests are dependent on UploadDataStream::OnReadCompleted(), which > > > should > > > > > > pass through the errors. > > > > > > > > > > That is why I added http stream parser changes into this cl. > > > > > > > > Unfortunately, this one can't land first - we need to keep Chrome > > continuously > > > > functional, since we ship it out nightly, and this breaks that. If we > move > > > the > > > > upload_data_stream.h / upload_data_stream.cc changes into the > > HttpStreamParser > > > > CL first, that's all that's needed to get the other CLs working, isn't it? > > > > > > Now that the other three have landed, LGTM! Don't forgot to merge with the > > > other landed cls (Or just remove the changes to everything but the > > > upload_elements_* files) > > > > How about the description? > > Good point. Maybe something like: > > Make UploadElementsDataStream return errors on failures. > > It previously zero-padded the response on errors, which had the > effect of uploading corrupted data, without indicating there was > a problem to consumers. > > Three earlier CLs upload consumers of this class to support this > new behavior: > > HttpStreamParser > https://codereview.chromium.org/2007013003/ > > SpdyHttpStream: > https://codereview.chromium.org/2022053002/ > > QuicHttpStream > https://codereview.chromium.org/2035643002/ Sounds good! Thank you!
The CQ bit was checked by maksim.sisov@intel.com
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/2030353002/#ps410001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030353002/410001
On 2016/06/23 16:05:37, mmenke wrote: > On 2016/06/23 16:02:49, maksims wrote: > > On 2016/06/23 15:07:18, mmenke wrote: > > > On 2016/06/22 20:24:07, mmenke wrote: > > > > On 2016/06/22 20:02:40, maksims wrote: > > > > > On 2016/06/22 20:00:40, maksims wrote: > > > > > > On 2016/06/22 19:26:56, mmenke wrote: > > > > > > > On 2016/06/22 19:12:20, maksims wrote: > > > > > > > > Yes, you are right. New patch without the check is uploaded. > > > > > > > > > > > > > > Looks good! Think it's about time to start landing these, following > > the > > > > > order > > > > > > I > > > > > > > suggested earlier. > > > > > > > > > > > > This one should land first, otherwise Http, Spdy and Quic just fail. > > Their > > > > > > unittests are dependent on UploadDataStream::OnReadCompleted(), which > > > should > > > > > > pass through the errors. > > > > > > > > > > That is why I added http stream parser changes into this cl. > > > > > > > > Unfortunately, this one can't land first - we need to keep Chrome > > continuously > > > > functional, since we ship it out nightly, and this breaks that. If we > move > > > the > > > > upload_data_stream.h / upload_data_stream.cc changes into the > > HttpStreamParser > > > > CL first, that's all that's needed to get the other CLs working, isn't it? > > > > > > Now that the other three have landed, LGTM! Don't forgot to merge with the > > > other landed cls (Or just remove the changes to everything but the > > > upload_elements_* files) > > > > How about the description? > > Good point. Maybe something like: > > Make UploadElementsDataStream return errors on failures. > > It previously zero-padded the response on errors, which had the > effect of uploading corrupted data, without indicating there was > a problem to consumers. > > Three earlier CLs upload consumers of this class to support this > new behavior: > > HttpStreamParser > https://codereview.chromium.org/2007013003/ > > SpdyHttpStream: > https://codereview.chromium.org/2022053002/ > > QuicHttpStream > https://codereview.chromium.org/2035643002/ Oops...That "upload consumers" should be "update consumers"
Message was sent while issue was closed.
Description was changed from ========== Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in ElementsUploadDataStream https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in ElementsUploadDataStream https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in ElementsUploadDataStream https://codereview.chromium.org/2030353002/ <------ current BUG=517615 ========== to ========== Make UploadElementsDataStream return errors on failures. It previously zero-padded the response on errors, which had the effect of uploading corrupted data, without indicating there was a problem to consumers. Three earlier CLs upload consumers of this class to support this new behavior: HttpStreamParser https://codereview.chromium.org/2007013003/ SpdyHttpStream: https://codereview.chromium.org/2022053002/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in ElementsUploadDataStream https://codereview.chromium.org/2030353002/ <------ current BUG=517615 Committed: https://crrev.com/e869bf5e1c460baf8a96f5b00407a76dad7e7ef8 Cr-Commit-Position: refs/heads/master@{#401636} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e869bf5e1c460baf8a96f5b00407a76dad7e7ef8 Cr-Commit-Position: refs/heads/master@{#401636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
