Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035643002:
Introduce error handling in QuicHttpStream 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce error handling in QuicHttpStream on
UploadDataStream::Read() failure.
QuicHttpStream 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 QuicHttpStream
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/
QuicHttpStream
https://codereview.chromium.org/2035643002/ <------ current
And the main change in UploadDataStream:
https://codereview.chromium.org/2030353002/
BUG=517615
Committed: https://crrev.com/84e20c9dfaf678e443f8ca7f5dea9a416fb55837
Cr-Commit-Position: refs/heads/master@{#401561}
Patch Set 1 #Patch Set 2 : removed test upload_data_stream file from the patch used to run try bots in patch 1 #
Total comments: 4
Patch Set 3 : #Patch Set 4 : rebasing #Patch Set 5 : remove 1 sec delay #Messages
Total messages: 20 (10 generated)
Description was changed from ========== quic handling errors BUG= ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
Might not be ideal, but would like to give it a try. please take a look
Description was changed from ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
mmenke@chromium.org changed reviewers: + rch@chromium.org
[+rch] https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:673: DoCallback(rv); I don't think you need to invoke the callback here - it looks like this class is designed to work much like HttpStreamParser, so I think you can just return rv here https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:680: return response_status_; This should go before the rv check - if this happens, the stream is already destroyed, so stream_->Reset will crash if you try to call it.
+1 to both of mmenke's comments
ptal https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:673: DoCallback(rv); On 2016/06/02 15:17:58, mmenke wrote: > I don't think you need to invoke the callback here - it looks like this class is > designed to work much like HttpStreamParser, so I think you can just return rv > here Done. https://codereview.chromium.org/2035643002/diff/20001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:680: return response_status_; On 2016/06/02 15:17:58, mmenke wrote: > This should go before the rv check - if this happens, the stream is already > destroyed, so stream_->Reset will crash if you try to call it. Done.
Description was changed from ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
LGTM! But please wait for Ryan's signoff before landing.
lgtm
removed 1 sec delay
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2035643002/#ps80001 (title: "remove 1 sec delay")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035643002/80001
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in QuicHttpStream on UploadDataStream::Read() failure. QuicHttpStream 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 QuicHttpStream 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/ QuicHttpStream https://codereview.chromium.org/2035643002/ <------ current And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 Committed: https://crrev.com/84e20c9dfaf678e443f8ca7f5dea9a416fb55837 Cr-Commit-Position: refs/heads/master@{#401561} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/84e20c9dfaf678e443f8ca7f5dea9a416fb55837 Cr-Commit-Position: refs/heads/master@{#401561} |
