Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863573007:
Reland Changed P2PTestConductor to use a separate worker thread (Closed)
|
Created:
4 years, 8 months ago by perkj_webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReland of Changed P2PTestConductor to use a separate worker thread.
patchset #1 contains the original cl.
https://codereview.webrtc.org/1859933002/
patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state.
The reason for the previous revert was:
Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg.
https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469/steps/peerconnection_unittests/logs/stdio
e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state()
Actual: 2
Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted
Which is: 3
BUG=webrtc:5426, webrtc:5752
Committed: https://crrev.com/8aba997f3ed3451d2f2b0359ce7c7161d747000f
Cr-Commit-Position: refs/heads/master@{#12309}
Patch Set 1 #Patch Set 2 : Changed to accept Connected and Completed #Patch Set 3 : Only accept kIceConnected in LocalP2PTestDtlsTransferCaller #Messages
Total messages: 19 (8 generated)
Description was changed from ========== Revert "Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1 id:1 of https://codereview.webrtc.org/1859933002/ )" This reverts commit 05255b0e8a4216a14e34b49ef8cbacb7defd8f3c. BUG= ========== to ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 ==========
Patchset #2 (id:20001) has been deleted
perkj@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Hi Can you please take a look at the original failor and see if this "fix" make sence or if there is a real bug in the ice states?
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org
On 2016/04/06 09:19:00, perkj_webrtc wrote: > Hi > Can you please take a look at the original failor and see if this "fix" make > sence or if there is a real bug in the ice states? This fix actually just hides a real issue with the ICE states. The test works by connecting to one peer, then switching to a different peer and doing an ICE restart. However, when the ICE restart occurs, the newly-gathered candidate is paired with the previous-generation candidate from the old connection. This pairing will take at least 10 seconds to time out, and prevents the state from going to "completed". I think that if an ICE restart occurs, and a generation "N" candidate pair becomes writable, it would make sense to discard the generation "< N" candidate pairs. Do you agree, Honghai?
On 2016/04/06 17:45:54, Taylor Brandstetter wrote: > On 2016/04/06 09:19:00, perkj_webrtc wrote: > > Hi > > Can you please take a look at the original failor and see if this "fix" make > > sence or if there is a real bug in the ice states? > > This fix actually just hides a real issue with the ICE states. > > The test works by connecting to one peer, then switching to a different peer and > doing an ICE restart. > > However, when the ICE restart occurs, the newly-gathered candidate is paired > with the previous-generation candidate from the old connection. This pairing > will take at least 10 seconds to time out, and prevents the state from going to > "completed". > > I think that if an ICE restart occurs, and a generation "N" candidate pair > becomes writable, it would make sense to discard the generation "< N" candidate > pairs. Do you agree, Honghai? Would you guys mind filing a bug for this that I can refer to in this cl and land this independently of the bug fix?
On 2016/04/07 06:06:11, perkj_webrtc wrote: > On 2016/04/06 17:45:54, Taylor Brandstetter wrote: > > On 2016/04/06 09:19:00, perkj_webrtc wrote: > > > Hi > > > Can you please take a look at the original failor and see if this "fix" make > > > sence or if there is a real bug in the ice states? > > > > This fix actually just hides a real issue with the ICE states. > > > > The test works by connecting to one peer, then switching to a different peer > and > > doing an ICE restart. > > > > However, when the ICE restart occurs, the newly-gathered candidate is paired > > with the previous-generation candidate from the old connection. This pairing > > will take at least 10 seconds to time out, and prevents the state from going > to > > "completed". > > > > I think that if an ICE restart occurs, and a generation "N" candidate pair > > becomes writable, it would make sense to discard the generation "< N" > candidate > > pairs. Do you agree, Honghai? > > Would you guys mind filing a bug for this that I can refer to in this cl and > land this independently of the bug fix? I'll file a bug after I do a bit more investigation. For now, in order to just fix the bot failures, can you just disable the failing test, as opposed to changing the "completed" check?
Thanks Ptal-
lgtm
On 2016/04/08 21:35:39, Taylor Brandstetter wrote: > lgtm Though it may look slightly nicer to replace the "bug5752" flag with a "waitForCompleted" flag, I don't care too much since it will be removed soon.
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863573007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863573007/60001
Message was sent while issue was closed.
Description was changed from ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 ========== to ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 ========== to ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 Committed: https://crrev.com/8aba997f3ed3451d2f2b0359ce7c7161d747000f Cr-Commit-Position: refs/heads/master@{#12309} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8aba997f3ed3451d2f2b0359ce7c7161d747000f Cr-Commit-Position: refs/heads/master@{#12309}
Message was sent while issue was closed.
Description was changed from ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426 Committed: https://crrev.com/8aba997f3ed3451d2f2b0359ce7c7161d747000f Cr-Commit-Position: refs/heads/master@{#12309} ========== to ========== Reland of Changed P2PTestConductor to use a separate worker thread. patchset #1 contains the original cl. https://codereview.webrtc.org/1859933002/ patchset #2 change the initiating client to accept both kIceConnectionCompleted kIceConnected as a ice state. The reason for the previous revert was: Causes P2PTestConductor.LocalP2PTestDtlsTransferCaller to fail on Win dbg. https://build.chromium.org/p/client.webrtc/builders/Win32%20Debug/builds/7469... e:\b\build\slave\win\build\src\webrtc\api\peerconnection_unittest.cc(1221): error: Value of: initiating_client_->ice_connection_state() Actual: 2 Expected: webrtc::PeerConnectionInterface::kIceConnectionCompleted Which is: 3 BUG= webrtc:5426, webrtc:5752 Committed: https://crrev.com/8aba997f3ed3451d2f2b0359ce7c7161d747000f Cr-Commit-Position: refs/heads/master@{#12309} ========== |
