Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3003633002:
ObjC: Always dispatch async in UIApplication status observer. (Closed)
DescriptionObjC: Always dispatch async in UIApplication status observer.
Fixes a possible deadlock.
BUG=webrtc:8130
Review-Url: https://codereview.webrtc.org/3003633002
Cr-Commit-Position: refs/heads/master@{#19464}
Committed: https://chromium.googlesource.com/external/webrtc/+/cca0006e33c106a7f62df1a1b50f84676cc5ed1e
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use timeout and RTC_CHECK. #
Total comments: 1
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by andersc@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
andersc@webrtc.org changed reviewers: + magjed@webrtc.org
Needed in order to enable some specific test on iOS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
andersc@webrtc.org changed reviewers: + denicija@webrtc.org, kthelgason@webrtc.org - magjed@webrtc.org
Changing reviewers
https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:64: dispatch_block_wait(_initializeBlock, DISPATCH_TIME_FOREVER); This will make the 'isApplicationActive' a blocking call. Will it have implications in other parts of the code?
https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:64: dispatch_block_wait(_initializeBlock, DISPATCH_TIME_FOREVER); On 2017/08/23 08:54:10, daniela-webrtc wrote: > This will make the 'isApplicationActive' a blocking call. Will it have > implications in other parts of the code? Before, we blocked on initialization by doing a dispatch_sync to the main queue, which was problematic. Now instead we block the calling queue if `isApplicationActive` is called before the ApplicationStatusObserver is fully initialized. It will block at most once, when calling it directly after initialization, before the _initializationBlock has completed, but some blocking is needed since we have to wait for the initialization to run on the main queue.
Seems well thought out on your part in that case. LGTM
neat! lgtm % comment https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:64: dispatch_block_wait(_initializeBlock, DISPATCH_TIME_FOREVER); On 2017/08/23 09:32:26, andersc wrote: > On 2017/08/23 08:54:10, daniela-webrtc wrote: > > This will make the 'isApplicationActive' a blocking call. Will it have > > implications in other parts of the code? > > Before, we blocked on initialization by doing a dispatch_sync to the main queue, > which was problematic. Now instead we block the calling queue if > `isApplicationActive` is called before the ApplicationStatusObserver is fully > initialized. It will block at most once, when calling it directly after > initialization, before the _initializationBlock has completed, but some blocking > is needed since we have to wait for the initialization to run on the main queue. Maybe instead of blocking forever we should add a timeout and a RTC_CHECK so that if this ever causes problems it will be easier to find than a deadlock.
https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3003633002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:64: dispatch_block_wait(_initializeBlock, DISPATCH_TIME_FOREVER); On 2017/08/23 11:37:39, kthelgason wrote: > On 2017/08/23 09:32:26, andersc wrote: > > On 2017/08/23 08:54:10, daniela-webrtc wrote: > > > This will make the 'isApplicationActive' a blocking call. Will it have > > > implications in other parts of the code? > > > > Before, we blocked on initialization by doing a dispatch_sync to the main > queue, > > which was problematic. Now instead we block the calling queue if > > `isApplicationActive` is called before the ApplicationStatusObserver is fully > > initialized. It will block at most once, when calling it directly after > > initialization, before the _initializationBlock has completed, but some > blocking > > is needed since we have to wait for the initialization to run on the main > queue. > > Maybe instead of blocking forever we should add a timeout and a RTC_CHECK so > that if this ever causes problems it will be easier to find than a deadlock. That sounds like a good idea! Done.
The CQ bit was checked by andersc@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andersc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/3003633002/#ps20001 (title: "Use timeout and RTC_CHECK.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1503493284007330,
"parent_rev": "a79cc28de1fd7f0fa8e331e9bf3d8bbe2a24fa23", "commit_rev":
"cca0006e33c106a7f62df1a1b50f84676cc5ed1e"}
Message was sent while issue was closed.
Description was changed from ========== ObjC: Always dispatch async in UIApplication status observer. Fixes a possible deadlock. BUG=webrtc:8130 ========== to ========== ObjC: Always dispatch async in UIApplication status observer. Fixes a possible deadlock. BUG=webrtc:8130 Review-Url: https://codereview.webrtc.org/3003633002 Cr-Commit-Position: refs/heads/master@{#19464} Committed: https://chromium.googlesource.com/external/webrtc/+/cca0006e33c106a7f62df1a1b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/cca0006e33c106a7f62df1a1b...
Message was sent while issue was closed.
https://codereview.webrtc.org/3003633002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3003633002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:68: RTC_CHECK_EQ(ret, 0); just a nit, but in the future be aware that the arguments to (D)CHECK_EQ are reversed, i.e. RTC_CHECK_EQ(0, ret) for some stupid reason. |
