Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124283003:
Fixing inconsistency with behavior of `ClearGettingPorts`. (Closed)
|
Created:
4 years, 5 months ago by Taylor Brandstetter Modified:
4 years, 4 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. |
DescriptionFixing inconsistency with behavior of `ClearGettingPorts`.
I found that, depending on when it's called, ClearGettingPorts may or
may not signal CandidatesAllocationDone, and may or may not continue
to gather more ports/candidates.
I'm fixing this inconsistency by having it always signal
CandidatesAllocationDone (if needed), and always stop gathering until
the next network change event. This makes it equivalent to
StopGettingPorts, except that it allows gathering to be restarted if
a network change occurs.
I also found that P2PTransportChannel was signaling "gathering
complete" even when continual gathering was enabled. This wasn't caught
by the unit tests due to the inconsistency of ClearGettingPorts as
described above.
Committed: https://crrev.com/b60a8198f1bca0f843743837e295f780c721f712
Cr-Commit-Position: refs/heads/master@{#13908}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Merging with master. #Patch Set 3 : Fixing test that tried to enable continual gathering in the middle of a session. #
Messages
Total messages: 27 (10 generated)
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
PTAL Peter & Honghai. https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { This class assumes the continual gathering policy doesn't change during the course of the session. So we should enforce that assumption.
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/07 17:52:37, Taylor Brandstetter wrote: > This class assumes the continual gathering policy doesn't change during the > course of the session. So we should enforce that assumption. If you set continual gathering policy after a session starts, it will have effect on the next session. It will also have effect on the current session unless the current session has been stopped or cleared. So I am not sure if we should enforce the assumption.
deadbeef@chromium.org changed reviewers: + deadbeef@chromium.org
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/21 04:10:59, honghaiz3 wrote: > It will also have effect on the current session unless the current session has > been stopped or cleared. This is the problem in my opinion. From an application's perspective it appears like a race condition; depending on when the policy is changed, it may or may not have an effect. Is there is a compelling use case for changing the gathering policy mid-session, we can allow it. But the behavior would need to be documented and covered sufficiently by unit tests.
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > On 2016/07/21 04:10:59, honghaiz3 wrote: > > It will also have effect on the current session unless the current session has > > been stopped or cleared. > > This is the problem in my opinion. From an application's perspective it appears > like a race condition; depending on when the policy is changed, it may or may > not have an effect. > > Is there is a compelling use case for changing the gathering policy mid-session, > we can allow it. But the behavior would need to be documented and covered > sufficiently by unit tests. In my opinion, the race condition is still there. You only changed it from "depending on whether the current session is cleared" to "depending on whether there is a session". We don't expect users to change it after session starts, it is just that I do not see the benefit of enforcing that assumption. It may happen that the user created the transport channel and set the config right away. If there is a pooled session, then the user may fail to set it. We can still print out the error message but don't have to stop the user from setting it. If you really want to enforce it, do so only if the current session is cleared.
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/21 17:37:25, honghaiz3 wrote: > On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > > On 2016/07/21 04:10:59, honghaiz3 wrote: > > > It will also have effect on the current session unless the current session > has > > > been stopped or cleared. > > > > This is the problem in my opinion. From an application's perspective it > appears > > like a race condition; depending on when the policy is changed, it may or may > > not have an effect. > > > > Is there is a compelling use case for changing the gathering policy > mid-session, > > we can allow it. But the behavior would need to be documented and covered > > sufficiently by unit tests. > > In my opinion, the race condition is still there. You only changed it from > "depending on whether the current session is cleared" to "depending on whether > there is a session". > > We don't expect users to change it after session starts, it is just that I do > not see the benefit of enforcing that assumption. > > It may happen that the user created the transport channel and set the config > right away. If there is a pooled session, then the user may fail to set it. > We can still print out the error message but don't have to stop the user from > setting it. > > If you really want to enforce it, do so only if the current session is cleared. Sessions, including pooled sessions, are only added in MaybeStartGathering, which is triggered by SetLocalDescription. So this condition should be equivalent to "don't allow the policy to change after SetLocalDescription is called". So I believe the behavior should be predictable. Also: the benefit of enforcing the assumption is that it prevents us from accidentally using a code path in the future that isn't covered by unit tests or intentionally supported.
lgtm. https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/21 18:38:38, Taylor Brandstetter wrote: > On 2016/07/21 17:37:25, honghaiz3 wrote: > > On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > > > On 2016/07/21 04:10:59, honghaiz3 wrote: > > > > It will also have effect on the current session unless the current session > > has > > > > been stopped or cleared. > > > > > > This is the problem in my opinion. From an application's perspective it > > appears > > > like a race condition; depending on when the policy is changed, it may or > may > > > not have an effect. > > > > > > Is there is a compelling use case for changing the gathering policy > > mid-session, > > > we can allow it. But the behavior would need to be documented and covered > > > sufficiently by unit tests. > > > > In my opinion, the race condition is still there. You only changed it from > > "depending on whether the current session is cleared" to "depending on whether > > there is a session". > > > > We don't expect users to change it after session starts, it is just that I do > > not see the benefit of enforcing that assumption. > > > > It may happen that the user created the transport channel and set the config > > right away. If there is a pooled session, then the user may fail to set it. > > We can still print out the error message but don't have to stop the user from > > setting it. > > > > If you really want to enforce it, do so only if the current session is > cleared. > > Sessions, including pooled sessions, are only added in MaybeStartGathering, > which is triggered by SetLocalDescription. So this condition should be > equivalent to "don't allow the policy to change after SetLocalDescription is > called". So I believe the behavior should be predictable. > > Also: the benefit of enforcing the assumption is that it prevents us from > accidentally using a code path in the future that isn't covered by unit tests or > intentionally supported. Can you please make a comment on the parameter continual_gathering_policy about the assumption that it must be set before SetLocalDescription and cannot be changed afterwards?
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > On 2016/07/21 04:10:59, honghaiz3 wrote: > > It will also have effect on the current session unless the current session has > > been stopped or cleared. > > This is the problem in my opinion. From an application's perspective it appears > like a race condition; depending on when the policy is changed, it may or may > not have an effect. > > Is there is a compelling use case for changing the gathering policy mid-session, > we can allow it. But the behavior would need to be documented and covered > sufficiently by unit tests. While I agree consistent behavior is good, it would be surprising to someone that call SetConfiguration with continual gathering to not get continual gathering and then get no error until they check the native log. If we're going to make the rule "you can't change the continual gathering setting", then we should cause SetConfiguration to fail, if that's even possible. Better would be to cause the current sessions to go into continual gathering mode. How hard would that be? https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/client/basicportal... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:268: network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP); ClearGettingPorts sends MSG_CONFIG_STOP? That's confusing. Should we call it MSG_CONFIG_CLEAR? Or should we call this StopGettingPortsUntilContinualGatheringTriggered?
https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/22 18:11:22, pthatcher1 wrote: > On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > > On 2016/07/21 04:10:59, honghaiz3 wrote: > > > It will also have effect on the current session unless the current session > has > > > been stopped or cleared. > > > > This is the problem in my opinion. From an application's perspective it > appears > > like a race condition; depending on when the policy is changed, it may or may > > not have an effect. > > > > Is there is a compelling use case for changing the gathering policy > mid-session, > > we can allow it. But the behavior would need to be documented and covered > > sufficiently by unit tests. > > While I agree consistent behavior is good, it would be surprising to someone > that call SetConfiguration with continual gathering to not get continual > gathering and then get no error until they check the native log. If we're going > to make the rule "you can't change the continual gathering setting", then we > should cause SetConfiguration to fail, if that's even possible. > > Better would be to cause the current sessions to go into continual gathering > mode. How hard would that be? JSEP already says that SetConfiguration only changes the ICE servers and filtering policy. Other changes are ignored, or in the case of changing certificates, an InvalidModificationError is thrown. I assumed we'd do one of those 2 things for our custom ICE options as well. As for "How hard would that be?": Like I said earlier, we'd need to document the various caveats and add unit tests for changing the policy at different points in time. This isn't trivial and I don't think it's worth the maintenance effort unless we see a compelling use case for changing the policy post-PC-creation. https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/client/basicportal... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:268: network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP); On 2016/07/22 18:11:22, pthatcher1 wrote: > ClearGettingPorts sends MSG_CONFIG_STOP? That's confusing. Should we call it > MSG_CONFIG_CLEAR? Or should we call this > StopGettingPortsUntilContinualGatheringTriggered? In my opinion the issue is more that "ClearGettingPorts" is not ideally named. "Stop/Clear" are actually more like "PermanentlyStop/Stop". But it would be difficult to change those names now since they're pure virtual and have overrides in downstream dependencies. By the way: Originally "ClearGettingPorts" was "StopGettingPorts(keep_session_running=true)" and it *did* post MSG_CONFIG_STOP: https://codereview.webrtc.org/1359363003/diff/120001/webrtc/p2p/client/basicp... But I think you requested 2 separate methods and MSG_CONFIG_STOP was unintentially lost as part of that refactoring.
lgtm I won't hold this up any more. https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2124283003/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:312: if (!allocator_sessions_.empty()) { On 2016/07/22 19:58:42, Taylor Brandstetter wrote: > On 2016/07/22 18:11:22, pthatcher1 wrote: > > On 2016/07/21 16:41:01, Taylor_Brandstetter wrote: > > > On 2016/07/21 04:10:59, honghaiz3 wrote: > > > > It will also have effect on the current session unless the current session > > has > > > > been stopped or cleared. > > > > > > This is the problem in my opinion. From an application's perspective it > > appears > > > like a race condition; depending on when the policy is changed, it may or > may > > > not have an effect. > > > > > > Is there is a compelling use case for changing the gathering policy > > mid-session, > > > we can allow it. But the behavior would need to be documented and covered > > > sufficiently by unit tests. > > > > While I agree consistent behavior is good, it would be surprising to someone > > that call SetConfiguration with continual gathering to not get continual > > gathering and then get no error until they check the native log. If we're > going > > to make the rule "you can't change the continual gathering setting", then we > > should cause SetConfiguration to fail, if that's even possible. > > > > Better would be to cause the current sessions to go into continual gathering > > mode. How hard would that be? > > JSEP already says that SetConfiguration only changes the ICE servers and > filtering policy. Other changes are ignored, or in the case of changing > certificates, an InvalidModificationError is thrown. I assumed we'd do one of > those 2 things for our custom ICE options as well. > > As for "How hard would that be?": Like I said earlier, we'd need to document the > various caveats and add unit tests for changing the policy at different points > in time. This isn't trivial and I don't think it's worth the maintenance effort > unless we see a compelling use case for changing the policy post-PC-creation. That's a good point about SetConfiguration only allowing ICE servers. You've convinced me.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2124283003/#ps20001 (title: "Merging with master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/17459)
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/2124283003/#ps40001 (title: "Fixing test that tried to enable continual gathering in the middle of a session.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fixing inconsistency with behavior of `ClearGettingPorts`. I found that, depending on when it's called, ClearGettingPorts may or may not signal CandidatesAllocationDone, and may or may not continue to gather more ports/candidates. I'm fixing this inconsistency by having it always signal CandidatesAllocationDone (if needed), and always stop gathering until the next network change event. This makes it equivalent to StopGettingPorts, except that it allows gathering to be restarted if a network change occurs. I also found that P2PTransportChannel was signaling "gathering complete" even when continual gathering was enabled. This wasn't caught by the unit tests due to the inconsistency of ClearGettingPorts as described above. ========== to ========== Fixing inconsistency with behavior of `ClearGettingPorts`. I found that, depending on when it's called, ClearGettingPorts may or may not signal CandidatesAllocationDone, and may or may not continue to gather more ports/candidates. I'm fixing this inconsistency by having it always signal CandidatesAllocationDone (if needed), and always stop gathering until the next network change event. This makes it equivalent to StopGettingPorts, except that it allows gathering to be restarted if a network change occurs. I also found that P2PTransportChannel was signaling "gathering complete" even when continual gathering was enabled. This wasn't caught by the unit tests due to the inconsistency of ClearGettingPorts as described above. Committed: https://crrev.com/b60a8198f1bca0f843743837e295f780c721f712 Cr-Commit-Position: refs/heads/master@{#13908} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b60a8198f1bca0f843743837e295f780c721f712 Cr-Commit-Position: refs/heads/master@{#13908} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
