Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years ago by honghaiz3 Modified:
4 years, 11 months ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't delete an ICE connection until it has been pruned or timed out on writing in the case where it
hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out
when it hasn't received anything yet leads to ICE connections being deleted
before they have a chance to send a ping and receive a response.
BUG=
Committed: https://crrev.com/37389b42b4f42c49583b5fa187611cdb6dcd67b4
Cr-Commit-Position: refs/heads/master@{#11151}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Fix comments #
Created: 4 years, 11 months ago
Messages
Total messages: 15 (7 generated)
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
I think the CL description could be described better. Perhaps something like: Don't delete an ICE connection until it has been pruned in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1135: } Let me see if I understand the purpose of this CL correctly: Currently, we are creating a connection, it's still actively pinging, but it hasn't received anything yet, and we decide it's dead, perhaps because it never had enough time to ping and receive something. In that case, I think something like this might be more readable: if (last_received() > 0) { // If it has ever received anything, we keep it alive until it hasn't received anything for DEAD_CONNECTION_RECEIVE_TIMEOUT // This covers the normal case of a successfully used connection that stops working. // This also allows a remote peer to continue pinging over a locally inactive (pruned) connection. return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)); } if (active()) { // If it has never received anything, but it is actively pinging (not pruned), // it is alive at least as long as it is pinging. Otherwise, the connection might be deleted // before it has a chance to ping. // This is the normal case for a new connection that is pinging but hasn't received anything yet. return false; } // if it has never received anything and it not actively pinging (pruned), but we keep it around at least MIN_CONNECTION_LIFETIME // to prevent connections from being pruned too quickly during a network change event when two networks would be up // simultaneously but only for a brief period. return now > (time_created_ms_ + MIN_CONNECTION_LIFETIME); https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:1263: // MIN_CONNECTION_LIFETIME AND inactive (write_timeout). This is now testing the case that when we are after the MIN_CONNECTION_LIFETIME and active, we don't kill the connection. But now we no longer test the case that when we are before the MIN_CONNECTION_LIFETIME and inactive, we don't kill the connection. We need to test both, right?
Description was changed from ========== Delete a connection after it becomes write_timeout if it has not received anything. This prevent it from being deleted too early. BUG= ========== to ========== Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. BUG= ==========
PTAL. Thanks! https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1135: } On 2015/12/30 17:12:28, pthatcher1 wrote: > Let me see if I understand the purpose of this CL correctly: Currently, we are > creating a connection, it's still actively pinging, but it hasn't received > anything yet, and we decide it's dead, perhaps because it never had enough time > to ping and receive something. > > In that case, I think something like this might be more readable: > > if (last_received() > 0) { > // If it has ever received anything, we keep it alive until it hasn't received > anything for DEAD_CONNECTION_RECEIVE_TIMEOUT > // This covers the normal case of a successfully used connection that stops > working. > // This also allows a remote peer to continue pinging over a locally inactive > (pruned) connection. > return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT)); > } > > if (active()) { > // If it has never received anything, but it is actively pinging (not pruned), > > // it is alive at least as long as it is pinging. Otherwise, the connection > might be deleted > // before it has a chance to ping. > // This is the normal case for a new connection that is pinging but hasn't > received anything yet. > return false; > } > > // if it has never received anything and it not actively pinging (pruned), but > we keep it around at least MIN_CONNECTION_LIFETIME > // to prevent connections from being pruned too quickly during a network change > event when two networks would be up > // simultaneously but only for a brief period. > return now > (time_created_ms_ + MIN_CONNECTION_LIFETIME); Yes. More precisely, having 10 seconds for the minimum lifetime is not enough in some cases. Some good connection may not have a chance to send a ping before it is deleted. This happened sometimes when both sides have two networks (so more connections are created). https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/1544003002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:1263: // MIN_CONNECTION_LIFETIME AND inactive (write_timeout). On 2015/12/30 17:12:28, pthatcher1 wrote: > This is now testing the case that when we are after the MIN_CONNECTION_LIFETIME > and active, we don't kill the connection. But now we no longer test the case > that when we are before the MIN_CONNECTION_LIFETIME and inactive, we don't kill > the connection. We need to test both, right? Done.
lgtm, with nit https://codereview.webrtc.org/1544003002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1544003002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1144: // if it has never received anything and it not actively pinging (pruned), we if => If
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1544003002/#ps60001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1544003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1544003002/60001
https://codereview.webrtc.org/1544003002/diff/40001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1544003002/diff/40001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:1144: // if it has never received anything and it not actively pinging (pruned), we On 2016/01/05 00:11:26, pthatcher1 wrote: > if => If Done.
Message was sent while issue was closed.
Description was changed from ========== Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. BUG= ========== to ========== Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. BUG= ========== to ========== Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out when it hasn't received anything yet leads to ICE connections being deleted before they have a chance to send a ping and receive a response. BUG= Committed: https://crrev.com/37389b42b4f42c49583b5fa187611cdb6dcd67b4 Cr-Commit-Position: refs/heads/master@{#11151} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/37389b42b4f42c49583b5fa187611cdb6dcd67b4 Cr-Commit-Position: refs/heads/master@{#11151} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
