Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022053002:
Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. (Closed)
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce error handling in SpdyHttpStream on
UploadDataStream::Read() failure.
SpdyHttpStream was written with the assumption
UploadDataStream::Read() never fails, which the
UploadDataStream API currently guarantees. Instead,
on read errors, UploadDataStream zero pads the
response, resulting in sending corrupted data. We're
working on changing the UploadDataStream API so that
Read will return errors on failures.
This cl introduces error handling in SpdyHttpStream
class after UploadDataStream::Read() starts returning
errors if read fails.
There are several reviews for 3 streams:
SpdyHttpStream:
https://codereview.chromium.org/2022053002/ <------ current
HttpStreamParser
https://codereview.chromium.org/2007013003/
QuicHttpStream
https://codereview.chromium.org/2035643002/
And the main change in UploadDataStream:
https://codereview.chromium.org/2030353002/
BUG=517615
Committed: https://crrev.com/167c3c06f04977980f7ec02bd9d98a45ac2e9e52
Cr-Commit-Position: refs/heads/master@{#401556}
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 4
Patch Set 5 : rebasing + comments #
Total comments: 10
Patch Set 6 : comments + moving ReadErrorUploadDataStream back to unnamed naspace #
Messages
Total messages: 47 (17 generated)
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: HttpStreamParser https://codereview.chromium.org/2007013003/ BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002 <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002 <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
I guess it can be a PTAL now :)
On 2016/05/31 10:31:46, maksims wrote: > I guess it can be a PTAL now :) Yesterday was a holiday in the US, and I may end up taking today off, so may not get to these until tomorrow - sorry for the delay.
mmenke@chromium.org changed reviewers: + bnc@chromium.org
[+bnc], who knows the HTTP2 (SPDY) code. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. We should not be invoking the callback twice, under any circumstances. We should actually change the code to only run the callback after the body has been sent. That's what the HttpStreamParser does as well. Weird that the behavior diverges here. Also unfortunate that our usual net/ callback test infrastructure doesn't catch unexpected callback invocations. :( https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:474: weak_factory_.GetWeakPtr())); Should do this first - invoking |request_callback_| may actually delete |this|. Could add a test for that case. (Didn't suggest it for HttpStreamParser because it shares code to get that behavior, while for this class, the paths have to invoke the callbacks themselves, since there's no DoLoop) https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:930: spdy_util_.ConstructChunkedSpdyPost(NULL, 0)); Note that my comments apply to both test. nit: nullptr. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:932: framer.CreateRstStream(1, RST_STREAM_INTERNAL_ERROR)); This should probably use spdy_util_.ConstructSpdyFrame, to match how the request is created. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:941: CreateMockRead(*resp, 2), CreateMockRead(*rst_frame, 3), Are these frames needed?
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
On 2016/06/01 17:24:07, mmenke wrote: > [+bnc], who knows the HTTP2 (SPDY) code. > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > File net/spdy/spdy_http_stream.cc (right): > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. > We should not be invoking the callback twice, under any circumstances. We > should actually change the code to only run the callback after the body has been > sent. That's what the HttpStreamParser does as well. Weird that the behavior > diverges here. > > Also unfortunate that our usual net/ callback test infrastructure doesn't catch > unexpected callback invocations. :( > The main question, why spdy is written in a different way? Can I modify the delegate that defines read and send body functions to return the result of operations like HttpStreamParser does? > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > net/spdy/spdy_http_stream.cc:474: weak_factory_.GetWeakPtr())); > Should do this first - invoking |request_callback_| may actually delete |this|. > Could add a test for that case. (Didn't suggest it for HttpStreamParser because > it shares code to get that behavior, while for this class, the paths have to > invoke the callbacks themselves, since there's no DoLoop) >
And as I see there is no any expectations that callback will be called after body is sent. Otherwise, there is a race condition and, for example, ConnectionClosedDuringChunkedPost test fails. Callback returns OK instead of ERR_CONNECTION_CLOSED
On 2016/06/02 12:34:06, maksims wrote: > And as I see there is no any expectations that callback will be called after > body is sent. > Otherwise, there is a race condition and, for example, > ConnectionClosedDuringChunkedPost test > fails. Callback returns OK instead of ERR_CONNECTION_CLOSED There are***
https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:474: weak_factory_.GetWeakPtr())); On 2016/06/01 17:24:07, mmenke wrote: > Should do this first - invoking |request_callback_| may actually delete |this|. > Could add a test for that case. (Didn't suggest it for HttpStreamParser because > it shares code to get that behavior, while for this class, the paths have to > invoke the callbacks themselves, since there's no DoLoop) Do you mean - base::WeakPtr<SpdyHttpStream> weak_ptr = weak_factory_.GetWeakPtr(); if (!request_callback_.is_null()) { .. } CHECK(weak_ptr); ? https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:941: CreateMockRead(*resp, 2), CreateMockRead(*rst_frame, 3), On 2016/06/01 17:24:07, mmenke wrote: > Are these frames needed? Otherwise I got !AllReadDataConsumed failures.
On 2016/06/02 12:04:52, maksims wrote: > On 2016/06/01 17:24:07, mmenke wrote: > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > File net/spdy/spdy_http_stream.cc (right): > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. > > We should not be invoking the callback twice, under any circumstances. We > > should actually change the code to only run the callback after the body has > been > > sent. That's what the HttpStreamParser does as well. Weird that the behavior > > diverges here. > > > > Also unfortunate that our usual net/ callback test infrastructure doesn't > catch > > unexpected callback invocations. :( > > > > The main question, why spdy is written in a different way? Can I modify the > delegate that > defines read and send body functions to return the result of operations like > HttpStreamParser does? I don't know (answer to both questions). I defer to bnc on both issues.
On 2016/06/02 12:34:21, maksims wrote: > On 2016/06/02 12:34:06, maksims wrote: > > And as I see there is no any expectations that callback will be called after > > body is sent. > > Otherwise, there is a race condition and, for example, > > ConnectionClosedDuringChunkedPost test > > fails. Callback returns OK instead of ERR_CONNECTION_CLOSED > > There are*** I'd say adjust the tests to fit the new behavior - the old behavior is inconsistent with HttpStreamParser, which is what the API was modelled after.
https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:474: weak_factory_.GetWeakPtr())); On 2016/06/02 12:43:12, maksims wrote: > On 2016/06/01 17:24:07, mmenke wrote: > > Should do this first - invoking |request_callback_| may actually delete > |this|. > > Could add a test for that case. (Didn't suggest it for HttpStreamParser > because > > it shares code to get that behavior, while for this class, the paths have to > > invoke the callbacks themselves, since there's no DoLoop) > Do you mean - > base::WeakPtr<SpdyHttpStream> weak_ptr = weak_factory_.GetWeakPtr(); > if (!request_callback_.is_null()) { > .. > } > CHECK(weak_ptr); > ? No, I mean do the PostTask to reset the stream, and then invoke the callback. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:941: CreateMockRead(*resp, 2), CreateMockRead(*rst_frame, 3), On 2016/06/02 12:43:12, maksims wrote: > On 2016/06/01 17:24:07, mmenke wrote: > > Are these frames needed? > > Otherwise I got !AllReadDataConsumed failures. Even if you just have the "MockRead(SYNCHRONOUS, 0, 2)" as the only read here?
On 2016/06/02 14:59:33, mmenke wrote: > On 2016/06/02 12:04:52, maksims wrote: > > On 2016/06/01 17:24:07, mmenke wrote: > > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > File net/spdy/spdy_http_stream.cc (right): > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. > > > We should not be invoking the callback twice, under any circumstances. We > > > should actually change the code to only run the callback after the body has > > been > > > sent. That's what the HttpStreamParser does as well. Weird that the > behavior > > > diverges here. > > > > > > Also unfortunate that our usual net/ callback test infrastructure doesn't > > catch > > > unexpected callback invocations. :( > > > > > > > The main question, why spdy is written in a different way? Can I modify the > > delegate that > > defines read and send body functions to return the result of operations like > > HttpStreamParser does? > > I don't know (answer to both questions). I defer to bnc on both issues. Actually, the issue may be that HTTP 2 streams can receive data while still sending the body, so the point is to allow the consumer to read data while still uploading the body. If so, we may actually need to send the error message to the body read request (If one is pending - if one is not pending, we'll need to send the error when it starts).
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. On 2016/06/01 17:24:07, mmenke wrote: > We should not be invoking the callback twice, under any circumstances. We > should actually change the code to only run the callback after the body has been > sent. That's what the HttpStreamParser does as well. Weird that the behavior > diverges here. > > Also unfortunate that our usual net/ callback test infrastructure doesn't catch > unexpected callback invocations. :( Done. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:474: weak_factory_.GetWeakPtr())); On 2016/06/02 15:02:10, mmenke wrote: > On 2016/06/02 12:43:12, maksims wrote: > > On 2016/06/01 17:24:07, mmenke wrote: > > > Should do this first - invoking |request_callback_| may actually delete > > |this|. > > > Could add a test for that case. (Didn't suggest it for HttpStreamParser > > because > > > it shares code to get that behavior, while for this class, the paths have to > > > invoke the callbacks themselves, since there's no DoLoop) > > Do you mean - > > base::WeakPtr<SpdyHttpStream> weak_ptr = weak_factory_.GetWeakPtr(); > > if (!request_callback_.is_null()) { > > .. > > } > > CHECK(weak_ptr); > > ? > > No, I mean do the PostTask to reset the stream, and then invoke the callback. Done. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:932: framer.CreateRstStream(1, RST_STREAM_INTERNAL_ERROR)); On 2016/06/01 17:24:07, mmenke wrote: > This should probably use spdy_util_.ConstructSpdyFrame, to match how the request > is created. spdy_util_.ConstructSpdyRstStream does the great job. Thanks. https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream_unittest.cc:941: CreateMockRead(*resp, 2), CreateMockRead(*rst_frame, 3), On 2016/06/02 15:02:10, mmenke wrote: > On 2016/06/02 12:43:12, maksims wrote: > > On 2016/06/01 17:24:07, mmenke wrote: > > > Are these frames needed? > > > > Otherwise I got !AllReadDataConsumed failures. > > Even if you just have the "MockRead(SYNCHRONOUS, 0, 2)" as the only read here? Oh, I got it. the resp frame is needed, but the rst_frame is not needed. https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:476: EXPECT_EQ(OK, callback.WaitForResult()); As I said this returns OK instead of ERR_CONNECTION_CLOSED due to new behavior. https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:888: EXPECT_EQ(OK, callback.WaitForResult()); Due to new behavior, the callback should be read only after chunked data is sent. Old behavior called the callback immediately after headers were sent.
On 2016/06/02 15:13:19, mmenke wrote: > On 2016/06/02 14:59:33, mmenke wrote: > > On 2016/06/02 12:04:52, maksims wrote: > > > On 2016/06/01 17:24:07, mmenke wrote: > > > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > File net/spdy/spdy_http_stream.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() method. > > > > We should not be invoking the callback twice, under any circumstances. We > > > > should actually change the code to only run the callback after the body > has > > > been > > > > sent. That's what the HttpStreamParser does as well. Weird that the > > behavior > > > > diverges here. > > > > > > > > Also unfortunate that our usual net/ callback test infrastructure doesn't > > > catch > > > > unexpected callback invocations. :( > > > > > > > > > > The main question, why spdy is written in a different way? Can I modify the > > > delegate that > > > defines read and send body functions to return the result of operations like > > > HttpStreamParser does? > > > > I don't know (answer to both questions). I defer to bnc on both issues. > > Actually, the issue may be that HTTP 2 streams can receive data while still > sending the body, so the point is to allow the consumer to read data while still > uploading the body. If so, we may actually need to send the error message to > the body read request (If one is pending - if one is not pending, we'll need to > send the error when it starts). Then calling the callback should be sufficient, shouldn't it? Line 486 in the patch set 4
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
On 2016/06/03 11:43:52, maksims wrote: > On 2016/06/02 15:13:19, mmenke wrote: > > On 2016/06/02 14:59:33, mmenke wrote: > > > On 2016/06/02 12:04:52, maksims wrote: > > > > On 2016/06/01 17:24:07, mmenke wrote: > > > > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > File net/spdy/spdy_http_stream.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() > method. > > > > > We should not be invoking the callback twice, under any circumstances. > We > > > > > should actually change the code to only run the callback after the body > > has > > > > been > > > > > sent. That's what the HttpStreamParser does as well. Weird that the > > > behavior > > > > > diverges here. > > > > > > > > > > Also unfortunate that our usual net/ callback test infrastructure > doesn't > > > > catch > > > > > unexpected callback invocations. :( > > > > > > > > > > > > > The main question, why spdy is written in a different way? Can I modify > the > > > > delegate that > > > > defines read and send body functions to return the result of operations > like > > > > HttpStreamParser does? > > > > > > I don't know (answer to both questions). I defer to bnc on both issues. > > > > Actually, the issue may be that HTTP 2 streams can receive data while still > > sending the body, so the point is to allow the consumer to read data while > still > > uploading the body. If so, we may actually need to send the error message to > > the body read request (If one is pending - if one is not pending, we'll need > to > > send the error when it starts). > > Then calling the callback should be sufficient, shouldn't it? Line 486 in the > patch set 4 Not if it's already been NULLed out. We should wait for bnc to weigh in. [Bence]: Ping!
On 2016/06/03 16:07:56, mmenke wrote: > On 2016/06/03 11:43:52, maksims wrote: > > On 2016/06/02 15:13:19, mmenke wrote: > > > On 2016/06/02 14:59:33, mmenke wrote: > > > > On 2016/06/02 12:04:52, maksims wrote: > > > > > On 2016/06/01 17:24:07, mmenke wrote: > > > > > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > > File net/spdy/spdy_http_stream.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() > > method. > > > > > > We should not be invoking the callback twice, under any circumstances. > > > We > > > > > > should actually change the code to only run the callback after the > body > > > has > > > > > been > > > > > > sent. That's what the HttpStreamParser does as well. Weird that the > > > > behavior > > > > > > diverges here. > > > > > > > > > > > > Also unfortunate that our usual net/ callback test infrastructure > > doesn't > > > > > catch > > > > > > unexpected callback invocations. :( > > > > > > > > > > > > > > > > The main question, why spdy is written in a different way? Can I modify > > the > > > > > delegate that > > > > > defines read and send body functions to return the result of operations > > like > > > > > HttpStreamParser does? > > > > > > > > I don't know (answer to both questions). I defer to bnc on both issues. > > > > > > Actually, the issue may be that HTTP 2 streams can receive data while still > > > sending the body, so the point is to allow the consumer to read data while > > still > > > uploading the body. If so, we may actually need to send the error message > to > > > the body read request (If one is pending - if one is not pending, we'll need > > to > > > send the error when it starts). > > > > Then calling the callback should be sufficient, shouldn't it? Line 486 in the > > patch set 4 > > Not if it's already been NULLed out. We should wait for bnc to weigh in. > > [Bence]: Ping! Bence is OOO until 20th of June. Should we wait or?
On 2016/06/06 09:10:14, maksims wrote: > On 2016/06/03 16:07:56, mmenke wrote: > > On 2016/06/03 11:43:52, maksims wrote: > > > On 2016/06/02 15:13:19, mmenke wrote: > > > > On 2016/06/02 14:59:33, mmenke wrote: > > > > > On 2016/06/02 12:04:52, maksims wrote: > > > > > > On 2016/06/01 17:24:07, mmenke wrote: > > > > > > > [+bnc], who knows the HTTP2 (SPDY) code. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > > > File net/spdy/spdy_http_stream.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2022053002/diff/40001/net/spdy/spdy_http_stre... > > > > > > > net/spdy/spdy_http_stream.cc:307: // OnRequestBodyReadCompleted() > > > method. > > > > > > > We should not be invoking the callback twice, under any > circumstances. > > > > > We > > > > > > > should actually change the code to only run the callback after the > > body > > > > has > > > > > > been > > > > > > > sent. That's what the HttpStreamParser does as well. Weird that > the > > > > > behavior > > > > > > > diverges here. > > > > > > > > > > > > > > Also unfortunate that our usual net/ callback test infrastructure > > > doesn't > > > > > > catch > > > > > > > unexpected callback invocations. :( > > > > > > > > > > > > > > > > > > > The main question, why spdy is written in a different way? Can I > modify > > > the > > > > > > delegate that > > > > > > defines read and send body functions to return the result of > operations > > > like > > > > > > HttpStreamParser does? > > > > > > > > > > I don't know (answer to both questions). I defer to bnc on both issues. > > > > > > > > Actually, the issue may be that HTTP 2 streams can receive data while > still > > > > sending the body, so the point is to allow the consumer to read data while > > > still > > > > uploading the body. If so, we may actually need to send the error message > > to > > > > the body read request (If one is pending - if one is not pending, we'll > need > > > to > > > > send the error when it starts). > > > > > > Then calling the callback should be sufficient, shouldn't it? Line 486 in > the > > > patch set 4 > > > > Not if it's already been NULLed out. We should wait for bnc to weigh in. > > > > [Bence]: Ping! > > Bence is OOO until 20th of June. Should we wait or? If it's not a problem with you, I'd say we should just wait. I don't think I'll have the time to dig through the SPDY code to undestand just what's going on and figure out a good solution here. When he gets back, I'll ask him about this in person, not sure why he hasn't responded here yet. Sorry for the delay.
I apologize for being so extremely slow to respond. SpdyHttpStreamTest.DataReadErrorAsynchronous and SpdyHttpStreamTest.DataReadErrorSynchronous both crash for me locally, please fix. Thank you. https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:96: base::TimeDelta::FromSeconds(1)); Is there a way to avoid time delay in tests? In general this is a bad idea, because it makes the test slow (for example, there was a huge net_unittest speedup recenly by removing 100 ms delay between mock IOs and migrating to a different mock data class), and fragile (is this test not going to fail when run with asan/valgrind or just in an environment with very heavy load?) at the same time.
On 2016/06/10 17:39:18, Bence OOO until June 20 wrote: > I apologize for being so extremely slow to respond. > > SpdyHttpStreamTest.DataReadErrorAsynchronous and > SpdyHttpStreamTest.DataReadErrorSynchronous both crash for me locally, please > fix. > > Thank you. > Hi, They crash because there is another CL that makes UploadDataStream to return errors. It is here https://codereview.chromium.org/2030353002/ . But then another problem happens in Spdy/SpdyNetworkTransactionTest.ResponseBeforePostCompletes. It just hangs on helper.WaitForHeaders() because after my fix the callback is not called after headers are sent, but after body is sent. > https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... > File net/spdy/spdy_http_stream_unittest.cc (right): > > https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... > net/spdy/spdy_http_stream_unittest.cc:96: base::TimeDelta::FromSeconds(1)); > Is there a way to avoid time delay in tests? In general this is a bad idea, > because it makes the test slow (for example, there was a huge net_unittest > speedup recenly by removing 100 ms delay between mock IOs and migrating to a > different mock data class), and fragile (is this test not going to fail when run > with asan/valgrind or just in an environment with very heavy load?) at the same > time. I will test another solution.
Hi, On 2016/06/10 17:46:32, maksims wrote: > > Hi, > > They crash because there is another CL that makes UploadDataStream to return > errors. > It is here https://codereview.chromium.org/2030353002/ . > Okay, indeed if I patch in both this CL and 2030353002, then said tests pass. Are you planning to land 2030353002 before this, or are you planning to get them reviewed separately but then squash them (together with the other two) and TBR? Just FYI, I think there is a dependend CL feature in Rietveld that allows you to run trybots and stuff, though I never used it myself. > But then another problem happens in > Spdy/SpdyNetworkTransactionTest.ResponseBeforePostCompletes. It just > hangs on helper.WaitForHeaders() because after my fix the callback is not called > after headers are sent, but > after body is sent. So are you planning to fix that as part of this CL?
On 2016/06/10 17:54:51, Bence OOO until June 20 wrote: > Hi, > > On 2016/06/10 17:46:32, maksims wrote: > > > > Hi, > > > > They crash because there is another CL that makes UploadDataStream to return > > errors. > > It is here https://codereview.chromium.org/2030353002/ . > > > > Okay, indeed if I patch in both this CL and 2030353002, then said tests pass. > Are you planning to land 2030353002 before this, or are you planning to get them > reviewed separately but then squash them (together with the other two) and TBR? > Just FYI, I think there is a dependend CL feature in Rietveld that allows you to > run trybots and stuff, though I never used it myself. > Yes, there is a test only CL - https://codereview.chromium.org/2037353002/ > > But then another problem happens in > > Spdy/SpdyNetworkTransactionTest.ResponseBeforePostCompletes. It just > > hangs on helper.WaitForHeaders() because after my fix the callback is not > called > > after headers are sent, but > > after body is sent. > > So are you planning to fix that as part of this CL? Well it depends on your answer to the changes. Just check previous messages from mmenke in this cl. There were some questions.
and question is why spdy is implemented in other way different from HttpStreamParser and QuicHttpStream. The last two have other behaviour and internal DoLoop which makes things easier.
rch: Do you happen to know why SpdyHttpStream calls the callback it got in SendRequest right after request headers are sent before request data are sent, but QuicHttpStream does not? (See comment #10.) Please make sure I'm not giving bad advice below. maksims: (1) I agree that HTTP/2 and HTTP/1.1 has different features so behavior might need to be different (comment #21), but in fact HTTP/2 and QUIC are very similar, and I am not aware of any reason for which SpdyHttpStream and QuicHttpStream should act differently in this respect. I think it would be a good idea to modify SpdyHttpStream to only call request_callback_ after data are uploaded. This is not exactly trivial, so it would need to be done in a separate CL. Just factor out some of your changes from this CL, like not calling DoRequestCallback() in OnRequestHeadersSent() but calling it OnRequestBodyReadCompleted(). SpdyNetworkTransactionTest.Post passes for me. As for SpdyNetworkTransactionTest.ResponseBeforePostcCompletes, you'll need to remove the help.WaitForHeaders() call and the check in the next line (also make sure to remove WaitForHeaders() method from NormalSpdyTransactionHelper since it's not called elsewhere), and add base::RunLoop()::RunUntilIdle(). You should be able to make other failing tests pass with similar changes. I guess this justifies this change going in a CL on its own. (2) I do not know why there is no DoLoop in SpdyHttpStream. If it makes your life easier, you are more then welcome to modify the internal implementation of SpdyHttpStream by adding a DoLoop. This should not change behavior at all, so I expect that tests would not need to modified for this. Again, this would need to be done in a separate CL. I think both of these potential CLs would be relatively simple and would greatly improve the codebase. I am not saying that I expect you to do either of them. I am simply saying that if you find either of these changes helpful or necessary for the current CL, then you should do them in separate CLs. Thank you.
On 2016/06/10 20:30:09, Bence OOO until June 20 wrote: > rch: Do you happen to know why SpdyHttpStream calls the callback it got in > SendRequest right after request headers are sent before request data are sent, > but QuicHttpStream does not? (See comment #10.) Please make sure I'm not > giving bad advice below. > > maksims: (1) I agree that HTTP/2 and HTTP/1.1 has different features so > behavior might need to be different (comment #21), but in fact HTTP/2 and QUIC > are very similar, and I am not aware of any reason for which SpdyHttpStream and > QuicHttpStream should act differently in this respect. I think it would be a > good idea to modify SpdyHttpStream to only call request_callback_ after data are > uploaded. This is not exactly trivial, so it would need to be done in a > separate CL. Just factor out some of your changes from this CL, like not > calling DoRequestCallback() in OnRequestHeadersSent() but calling it > OnRequestBodyReadCompleted(). SpdyNetworkTransactionTest.Post passes for me. > As for SpdyNetworkTransactionTest.ResponseBeforePostcCompletes, you'll need to > remove the help.WaitForHeaders() call and the check in the next line (also make > sure to remove WaitForHeaders() method from NormalSpdyTransactionHelper since > it's not called elsewhere), and add base::RunLoop()::RunUntilIdle(). You should > be able to make other failing tests pass with similar changes. I guess this > justifies this change going in a CL on its own. > Ok, thank you for your answer! > (2) I do not know why there is no DoLoop in SpdyHttpStream. If it makes your > life easier, you are more then welcome to modify the internal implementation of > SpdyHttpStream by adding a DoLoop. This should not change behavior at all, so I > expect that tests would not need to modified for this. Again, this would need > to be done in a separate CL. > Yes, sure, I guess it would make life of other people easier as well when the newcomers will start to touch the code. > I think both of these potential CLs would be relatively simple and would greatly > improve the codebase. I am not saying that I expect you to do either of them. > I am simply saying that if you find either of these changes helpful or necessary > for the current CL, then you should do them in separate CLs. > > Thank you. Let's complete this set of CL's first and then start another project. Thank you for your piece of advice!
As long as the issue with callbacks is solved in another CL (https://codereview.chromium.org/2064593002/), please take a look into this one. https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:96: base::TimeDelta::FromSeconds(1)); On 2016/06/10 17:39:17, Bence OOO until June 20 wrote: > Is there a way to avoid time delay in tests? In general this is a bad idea, > because it makes the test slow (for example, there was a huge net_unittest > speedup recenly by removing 100 ms delay between mock IOs and migrating to a > different mock data class), and fragile (is this test not going to fail when run > with asan/valgrind or just in an environment with very heavy load?) at the same > time. Done.
LGTM modulo nits. Things are a lot cleaner now that you split the callback change into its own CL, thank you. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:472: // Call the |request_callback| with received error. Please remove this trivial comment line. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.h (right): https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.h:99: // Helper function used for resetting stream from inside the stream Please end this sentence with a period. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:988: // Server receives RST_STREAM_INTERNAL_ERROR on clients' internal failure. Are you talking about only one client? In that case, s/clients'/client's/. Also in the other test. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:990: // UploadDataStream::Read() Please end this sentence with a period. Also in the other test. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:1010: EXPECT_EQ(spdy_util_.spdy_version(), session_->GetProtocolVersion()); Add empty line after this line just like in the other test.
Just one comment. LGTM, otherwise (Deferring to Bence on tests) https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:77: } // namespace Why did you remove ReadErrorUploadDataStream from the anonymous namespace?
https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:77: } // namespace On 2016/06/20 15:23:42, mmenke wrote: > Why did you remove ReadErrorUploadDataStream from the anonymous namespace? By accident after rebasing. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:988: // Server receives RST_STREAM_INTERNAL_ERROR on clients' internal failure. On 2016/06/20 13:55:30, Bence wrote: > Are you talking about only one client? In that case, s/clients'/client's/. > Also in the other test. Done. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:990: // UploadDataStream::Read() On 2016/06/20 13:55:30, Bence wrote: > Please end this sentence with a period. Also in the other test. Done. https://codereview.chromium.org/2022053002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:1010: EXPECT_EQ(spdy_util_.spdy_version(), session_->GetProtocolVersion()); On 2016/06/20 13:55:30, Bence wrote: > Add empty line after this line just like in the other test. Done.
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2022053002/#ps140001 (title: "comments + moving ReadErrorUploadDataStream back to unnamed naspace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022053002/140001
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in SpdyHttpStream on UploadDataStream::Read() failure. SpdyHttpStream was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in SpdyHttpStream class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 Committed: https://crrev.com/167c3c06f04977980f7ec02bd9d98a45ac2e9e52 Cr-Commit-Position: refs/heads/master@{#401556} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/167c3c06f04977980f7ec02bd9d98a45ac2e9e52 Cr-Commit-Position: refs/heads/master@{#401556} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
