Issue 1866503003: Revert of Changed P2PTestConductor to use a separate WorkerThread.

Issue 1866503003: Revert of Changed P2PTestConductor to use a separate WorkerThread. (Closed)

Created:
4 years, 8 months ago by perkj_webrtc

Modified:
4 years, 8 months ago

Reviewers:
nisse-webrtc, Taylor Brandstetter, pthatcher1

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.

More Reviews

Description

Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1 id:1 of https://codereview.webrtc.org/1859933002/ ) Reason for revert: 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 Original issue's description: > Changed P2PTestConductor to use a separate WorkerThread. > > P2PTestConductor currently use the current thread both as a signaling thread and a worker thread. Although convenient while debugging, it can also hide real bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the worker thread is deadlocked in the track proxy due to that the worker thread waits for the signaling thread but the proxy in turns invokes the worker thread..... That bug was only discovered on Android. I suggest we let the P2PTestConductor use a separate thread as a worker thread to better cover how PeerConnections are used in reality. > > BUG=webrtc:5426 > > Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4 > Cr-Commit-Position: refs/heads/master@{#12252} TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5426 Committed: https://crrev.com/05255b0e8a4216a14e34b49ef8cbacb7defd8f3c Cr-Commit-Position: refs/heads/master@{#12255}

Patch Set 1 #

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+21 lines, -32 lines)			Patch
	M	webrtc/api/peerconnection_unittest.cc	View		10 chunks	+21 lines, -32 lines	0 comments	Download

Messages

Total messages: 6 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

perkj_webrtc

Created Revert of Changed P2PTestConductor to use a separate WorkerThread.

4 years, 8 months ago (2016-04-06 08:28:25 UTC) #1

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866503003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866503003/1

4 years, 8 months ago (2016-04-06 08:28:28 UTC) #2

commit-bot: I haz the power

Description was changed from ========== Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset ...

4 years, 8 months ago (2016-04-06 08:28:33 UTC) #3

Message was sent while issue was closed.

Description was changed from

==========
Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1
id:1 of https://codereview.webrtc.org/1859933002/ )

Reason for revert:
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




Original issue's description:
> Changed P2PTestConductor to use a separate WorkerThread.
> 
> P2PTestConductor currently use the current thread both as a signaling thread
and a worker thread. Although convenient while debugging, it can also hide real
bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the
worker thread is deadlocked in the track proxy due to that the worker thread
waits for the signaling thread but the proxy in turns invokes the worker
thread..... That bug was only discovered on Android. I suggest we let the
P2PTestConductor use a separate thread as a worker thread to better cover how
PeerConnections are used in reality.
> 
> BUG=webrtc:5426
> 
> Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4
> Cr-Commit-Position: refs/heads/master@{#12252}

TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5426
==========

to

==========
Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1
id:1 of https://codereview.webrtc.org/1859933002/ )

Reason for revert:
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




Original issue's description:
> Changed P2PTestConductor to use a separate WorkerThread.
> 
> P2PTestConductor currently use the current thread both as a signaling thread
and a worker thread. Although convenient while debugging, it can also hide real
bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the
worker thread is deadlocked in the track proxy due to that the worker thread
waits for the signaling thread but the proxy in turns invokes the worker
thread..... That bug was only discovered on Android. I suggest we let the
P2PTestConductor use a separate thread as a worker thread to better cover how
PeerConnections are used in reality.
> 
> BUG=webrtc:5426
> 
> Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4
> Cr-Commit-Position: refs/heads/master@{#12252}

TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5426
==========

commit-bot: I haz the power

Description was changed from ========== Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset ...

4 years, 8 months ago (2016-04-06 08:28:38 UTC) #5

Message was sent while issue was closed.

Description was changed from

==========
Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1
id:1 of https://codereview.webrtc.org/1859933002/ )

Reason for revert:
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




Original issue's description:
> Changed P2PTestConductor to use a separate WorkerThread.
> 
> P2PTestConductor currently use the current thread both as a signaling thread
and a worker thread. Although convenient while debugging, it can also hide real
bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the
worker thread is deadlocked in the track proxy due to that the worker thread
waits for the signaling thread but the proxy in turns invokes the worker
thread..... That bug was only discovered on Android. I suggest we let the
P2PTestConductor use a separate thread as a worker thread to better cover how
PeerConnections are used in reality.
> 
> BUG=webrtc:5426
> 
> Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4
> Cr-Commit-Position: refs/heads/master@{#12252}

TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5426
==========

to

==========
Revert of Changed P2PTestConductor to use a separate WorkerThread. (patchset #1
id:1 of https://codereview.webrtc.org/1859933002/ )

Reason for revert:
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

Original issue's description:
> Changed P2PTestConductor to use a separate WorkerThread.
>
> P2PTestConductor currently use the current thread both as a signaling thread
and a worker thread. Although convenient while debugging, it can also hide real
bugs. An example is https://codereview.webrtc.org/1766653002/#ps420001 where the
worker thread is deadlocked in the track proxy due to that the worker thread
waits for the signaling thread but the proxy in turns invokes the worker
thread..... That bug was only discovered on Android. I suggest we let the
P2PTestConductor use a separate thread as a worker thread to better cover how
PeerConnections are used in reality.
>
> BUG=webrtc:5426
>
> Committed: https://crrev.com/6172401972c54813698d73580779d675d99178b4
> Cr-Commit-Position: refs/heads/master@{#12252}

TBR=nisse@webrtc.org,pthatcher@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5426

Committed: https://crrev.com/05255b0e8a4216a14e34b49ef8cbacb7defd8f3c
Cr-Commit-Position: refs/heads/master@{#12255}
==========

commit-bot: I haz the power

4 years, 8 months ago (2016-04-06 08:28:41 UTC) #6

Message was sent while issue was closed.

Patchset 1 (id:??) landed as
https://crrev.com/05255b0e8a4216a14e34b49ef8cbacb7defd8f3c
Cr-Commit-Position: refs/heads/master@{#12255}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages