Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1307083002:
TCPConnection can never be deteted if they fail to connect (Closed)
|
Created:
5 years, 4 months ago by guoweis_webrtc Modified:
5 years, 4 months ago 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. |
DescriptionTCPConnection can never be deteted if they fail to connect.
Since the TCPConnection has never been connected, they are not scheduled for ping hence will never be detected.
Also fix the case when reconnect fails, as it has become READABLE before, it also will not be deleted.
BUG=webrtc:4936
R=pthatcher@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/1eb87c7d94916a6e5641bfbbc8df37527712e4ca
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #Messages
Total messages: 27 (13 generated)
guoweis@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
Please take a look before I start the test cases.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.cc File webrtc/p2p/base/tcpport.cc (right): https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:382: // Destroy() to ensure this connection doesn't linger. It seems like we should call OnClose here. Is there a reason not to? https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:406: Destroy(); I don't think this matches the state diagram at the top. It says if we're not connect and get OnClose, we should wait for a timeout before dying. Is the diagram wrong, the code wrong, or am I wrong? https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:418: // has been READABLE. This part seems correct. It matches the state diagram ("the connection is dead"), so I don't think you need the comment.
PTAL. Need to add couple test cases. 1. the case where Chrome calls OnClose without destroying the connection. 2. the case where the connection is deleted if it never connect 3. the case where reconnection fails, the connection is deleted. https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.cc File webrtc/p2p/base/tcpport.cc (right): https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:382: // Destroy() to ensure this connection doesn't linger. On 2015/08/22 05:05:22, pthatcher1 wrote: > It seems like we should call OnClose here. Is there a reason not to? Done. https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:406: Destroy(); On 2015/08/22 05:05:22, pthatcher1 wrote: > I don't think this matches the state diagram at the top. It says if we're not > connect and get OnClose, we should wait for a timeout before dying. Is the > diagram wrong, the code wrong, or am I wrong? > the diagram above misses an arrow from state 1 to state 7. I'll update that. https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:418: // has been READABLE. On 2015/08/22 05:05:22, pthatcher1 wrote: > This part seems correct. It matches the state diagram ("the connection is > dead"), so I don't think you need the comment. Will remove.
On 2015/08/22 05:29:50, guoweis wrote: > PTAL. > > Need to add couple test cases. > > 1. the case where Chrome calls OnClose without destroying the connection. > 2. the case where the connection is deleted if it never connect > 3. the case where reconnection fails, the connection is deleted. > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.cc > File webrtc/p2p/base/tcpport.cc (right): > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > webrtc/p2p/base/tcpport.cc:382: // Destroy() to ensure this connection doesn't > linger. > On 2015/08/22 05:05:22, pthatcher1 wrote: > > It seems like we should call OnClose here. Is there a reason not to? > > Done. > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > webrtc/p2p/base/tcpport.cc:406: Destroy(); > On 2015/08/22 05:05:22, pthatcher1 wrote: > > I don't think this matches the state diagram at the top. It says if we're not > > connect and get OnClose, we should wait for a timeout before dying. Is the > > diagram wrong, the code wrong, or am I wrong? > > > > the diagram above misses an arrow from state 1 to state 7. I'll update that. > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > webrtc/p2p/base/tcpport.cc:418: // has been READABLE. > On 2015/08/22 05:05:22, pthatcher1 wrote: > > This part seems correct. It matches the state diagram ("the connection is > > dead"), so I don't think you need the comment. > > Will remove. I'll start to update the test case today if there is no objection on how this is going.
On 2015/08/24 14:11:02, guoweis wrote: > On 2015/08/22 05:29:50, guoweis wrote: > > PTAL. > > > > Need to add couple test cases. > > > > 1. the case where Chrome calls OnClose without destroying the connection. > > 2. the case where the connection is deleted if it never connect > > 3. the case where reconnection fails, the connection is deleted. > > > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.cc > > File webrtc/p2p/base/tcpport.cc (right): > > > > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > > webrtc/p2p/base/tcpport.cc:382: // Destroy() to ensure this connection doesn't > > linger. > > On 2015/08/22 05:05:22, pthatcher1 wrote: > > > It seems like we should call OnClose here. Is there a reason not to? > > > > Done. > > > > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > > webrtc/p2p/base/tcpport.cc:406: Destroy(); > > On 2015/08/22 05:05:22, pthatcher1 wrote: > > > I don't think this matches the state diagram at the top. It says if we're > not > > > connect and get OnClose, we should wait for a timeout before dying. Is the > > > diagram wrong, the code wrong, or am I wrong? > > > > > > > the diagram above misses an arrow from state 1 to state 7. I'll update that. > > > > > https://codereview.webrtc.org/1307083002/diff/40001/webrtc/p2p/base/tcpport.c... > > webrtc/p2p/base/tcpport.cc:418: // has been READABLE. > > On 2015/08/22 05:05:22, pthatcher1 wrote: > > > This part seems correct. It matches the state diagram ("the connection is > > > dead"), so I don't think you need the comment. > > > > Will remove. > > I'll start to update the test case today if there is no objection on how this is > going. If fine that as long as the unit tests match the state diagram :).
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307083002/140001
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
PTAL. Added test cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307083002/140001
Patchset #4 (id:120002) has been deleted
The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307083002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307083002/170001
Patchset #3 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
lgtm, assuming we fix the comments https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... webrtc/p2p/base/port_unittest.cc:603: // Ensure redundant of OnClose on TcpConnection won't break tcp redundant of OnClose on TcpConnection => redundant SignalClose events https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... webrtc/p2p/base/port_unittest.cc:605: // during reconnection. Chromium calls OnClose or fires SignalClose? OnClose is private, right? https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/tcpport.cc File webrtc/p2p/base/tcpport.cc (right): https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/tcpport.... webrtc/p2p/base/tcpport.cc:400: // Prevent the connection to be destroyed. to be destroyed => from being destroyed
Message was sent while issue was closed.
Committed patchset #4 (id:190001) manually as 1eb87c7d94916a6e5641bfbbc8df37527712e4ca (presubmit successful).
Message was sent while issue was closed.
landed. https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... webrtc/p2p/base/port_unittest.cc:603: // Ensure redundant of OnClose on TcpConnection won't break tcp On 2015/08/25 17:58:41, pthatcher1 wrote: > redundant of OnClose on TcpConnection => redundant SignalClose events Done. https://codereview.webrtc.org/1307083002/diff/170001/webrtc/p2p/base/port_uni... webrtc/p2p/base/port_unittest.cc:605: // during reconnection. On 2015/08/25 17:58:41, pthatcher1 wrote: > Chromium calls OnClose or fires SignalClose? OnClose is private, right? Done. |
