Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2313053002:
Limits only 16 new QUIC connections can be opened per socket event for QuicSimpleServer. Fix test f… (Closed)
|
Created:
4 years, 3 months ago by danzh1 Modified:
4 years, 3 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLimits only 16 new QUIC connections can be opened per socket event for QuicSimpleServer. Fix test failure in https://codereview.chromium.org/2310543002/ which missed necessary changes to QuicSimpleServer.
This is a continuation of merging internal change: 131102299
R=rch@chromium.org
BUG=
Committed: https://crrev.com/2a5b9a61e64f1d27400ea6a0496bfbdba749ebca
Cr-Commit-Position: refs/heads/master@{#417099}
Patch Set 1 #
Total comments: 3
Patch Set 2 : don't reset read_pending_ while wait async read #
Total comments: 1
Patch Set 3 : use synchronous_read_count_ as indicator of new read loop #
Total comments: 3
Patch Set 4 : refine comment #
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by danzh@chromium.org
The CQ bit was unchecked by danzh@chromium.org
The CQ bit was unchecked by danzh@chromium.org
The CQ bit was checked by danzh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... File net/tools/quic/quic_simple_server.cc (right): https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... net/tools/quic/quic_simple_server.cc:171: read_pending_ = false; This looks like a bug. read_pending_ should be true until OnReadComplete() is called. Otherwise we can end up with multiple reads happening at once. https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... net/tools/quic/quic_simple_server.cc:175: weak_factory_.GetWeakPtr())); I don't think this is the right method to call. This will end up issuing another read to the socket, which will crash. I think you're going to need to split reading into two phases: 1) ProcessBufferedPackets 2) ReadPacketsFrom the network When we're done processing buffered packets, if there are still more buffered packets, we can post a task to process more buffered packets. When we've hit ERR_IO_PENDING, we can post a task to process more buffered packets. But we can not read more packets from the network until the last read completed because of how chrome sockets work. https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... net/tools/quic/quic_simple_server.cc:189: // No need to check buffer befor following read. nit: "before" not "befor" That being said, I'm sure sure that this comment add value. We've just read a packet and need to process it, so calling OnReadComplete seems like the right choice. It's not obvious to me why we might consider checking the buffer here. (If we needed to do that, I would think we would have done that before calling read)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danzh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/06 21:18:33, Ryan Hamilton wrote: > https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... > File net/tools/quic/quic_simple_server.cc (right): > > https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... > net/tools/quic/quic_simple_server.cc:171: read_pending_ = false; > This looks like a bug. read_pending_ should be true until OnReadComplete() is > called. Otherwise we can end up with multiple reads happening at once. > > https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... > net/tools/quic/quic_simple_server.cc:175: weak_factory_.GetWeakPtr())); > I don't think this is the right method to call. This will end up issuing another > read to the socket, which will crash. > > I think you're going to need to split reading into two phases: > > 1) ProcessBufferedPackets > 2) ReadPacketsFrom the network > > When we're done processing buffered packets, if there are still more buffered > packets, we can post a task to process more buffered packets. When we've hit > ERR_IO_PENDING, we can post a task to process more buffered packets. > > But we can not read more packets from the network until the last read completed > because of how chrome sockets work. > > https://codereview.chromium.org/2313053002/diff/1/net/tools/quic/quic_simple_... > net/tools/quic/quic_simple_server.cc:189: // No need to check buffer befor > following read. > nit: "before" not "befor" > > That being said, I'm sure sure that this comment add value. We've just read a > packet and need to process it, so calling OnReadComplete seems like the right > choice. It's not obvious to me why we might consider checking the buffer here. > (If we needed to do that, I would think we would have done that before calling > read) I changed the way how it posts extra socket event. Every async read will register to check buffered packets. PTAL!
https://codereview.chromium.org/2313053002/diff/20001/net/tools/quic/quic_sim... File net/tools/quic/quic_simple_server.cc (right): https://codereview.chromium.org/2313053002/diff/20001/net/tools/quic/quic_sim... net/tools/quic/quic_simple_server.cc:151: if (check_packet_buffer) { I wonder if you could simply check synchronous_read_count_ == 0 here instead, since that's only true once per event loop? If so, that'd be pretty simple.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danzh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/06 23:21:46, Ryan Hamilton wrote: > https://codereview.chromium.org/2313053002/diff/20001/net/tools/quic/quic_sim... > File net/tools/quic/quic_simple_server.cc (right): > > https://codereview.chromium.org/2313053002/diff/20001/net/tools/quic/quic_sim... > net/tools/quic/quic_simple_server.cc:151: if (check_packet_buffer) { > I wonder if you could simply check synchronous_read_count_ == 0 here instead, > since that's only true once per event loop? If so, that'd be pretty simple. done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sweet! Looking good. https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... File net/tools/quic/quic_simple_server.cc (right): https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... net/tools/quic/quic_simple_server.cc:151: if (synchronous_read_count_ == 0) { If you wanted, you could add a comment here like: // Only process buffered packets once per message loop. https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... net/tools/quic/quic_simple_server.cc:168: // next socket event if there is any. nit: Can you shrink this comment to: // No more packets to read, so yield before processing buffered packets. https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... net/tools/quic/quic_simple_server.cc:177: // yeild and continue processing in next event. I'd just drop this comment (and the next two) since they're not super helpful. (And this comment mentions "server" but I think it's actually referring to the comment.
On 2016/09/07 19:46:18, Ryan Hamilton wrote: > Sweet! Looking good. > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > File net/tools/quic/quic_simple_server.cc (right): > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > net/tools/quic/quic_simple_server.cc:151: if (synchronous_read_count_ == 0) { > If you wanted, you could add a comment here like: > > // Only process buffered packets once per message loop. > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > net/tools/quic/quic_simple_server.cc:168: // next socket event if there is any. > nit: Can you shrink this comment to: > > // No more packets to read, so yield before processing buffered packets. > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > net/tools/quic/quic_simple_server.cc:177: // yeild and continue processing in > next event. > I'd just drop this comment (and the next two) since they're not super helpful. > (And this comment mentions "server" but I think it's actually referring to the > comment.
On 2016/09/07 21:13:33, danzh1 wrote: > On 2016/09/07 19:46:18, Ryan Hamilton wrote: > > Sweet! Looking good. > > > > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > > File net/tools/quic/quic_simple_server.cc (right): > > > > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > > net/tools/quic/quic_simple_server.cc:151: if (synchronous_read_count_ == 0) { > > If you wanted, you could add a comment here like: > > > > // Only process buffered packets once per message loop. > > > > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > > net/tools/quic/quic_simple_server.cc:168: // next socket event if there is > any. > > nit: Can you shrink this comment to: > > > > // No more packets to read, so yield before processing buffered packets. > > > > > https://codereview.chromium.org/2313053002/diff/40001/net/tools/quic/quic_sim... > > net/tools/quic/quic_simple_server.cc:177: // yeild and continue processing in > > next event. > > I'd just drop this comment (and the next two) since they're not super helpful. > > (And this comment mentions "server" but I think it's actually referring to the > > comment. refined all comments
The CQ bit was checked by rch@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Limits only 16 new QUIC connections can be opened per socket event for QuicSimpleServer. Fix test failure in https://codereview.chromium.org/2310543002/ which missed necessary changes to QuicSimpleServer. This is a continuation of merging internal change: 131102299 R=rch@chromium.org BUG= ========== to ========== Limits only 16 new QUIC connections can be opened per socket event for QuicSimpleServer. Fix test failure in https://codereview.chromium.org/2310543002/ which missed necessary changes to QuicSimpleServer. This is a continuation of merging internal change: 131102299 R=rch@chromium.org BUG= Committed: https://crrev.com/2a5b9a61e64f1d27400ea6a0496bfbdba749ebca Cr-Commit-Position: refs/heads/master@{#417099} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2a5b9a61e64f1d27400ea6a0496bfbdba749ebca Cr-Commit-Position: refs/heads/master@{#417099} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
