Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859933002:
Changed P2PTestConductor to use a separate WorkerThread. (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. |
DescriptionChanged 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}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 16 (7 generated)
The CQ bit was checked by perkj@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/1859933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859933002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
perkj@webrtc.org changed reviewers: + deadbeef@webrtc.org, nisse@webrtc.org, pthatcher@webrtc.org
wdyt?
lgtm https://codereview.webrtc.org/1859933002/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1859933002/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:1306: // |receiving_client_|. Must be destroyed last. Relying on the order of instance variables?
Description was changed from ========== 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 ========== to ========== 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 ==========
lgtm Maybe peerconnectionendtoend_unittest should use a worker thread as well?
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/1859933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859933002/1
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6172401972c54813698d73580779d675d99178b4 Cr-Commit-Position: refs/heads/master@{#12252}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.webrtc.org/1866503003/ by perkj@webrtc.org. The reason for reverting is: 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 . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
