|
|
Created:
3 years, 6 months ago by Roger Tawa OOO till Jul 10th Modified:
3 years, 4 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove call to GetBlockingPool in RLZ.
BUG=667892, 618530
Review-Url: https://codereview.chromium.org/2949263003
Cr-Commit-Position: refs/heads/master@{#491013}
Committed: https://chromium.googlesource.com/chromium/src/+/c4a9c77f6e6c59b02fbb09db8da441f630b9ac54
Patch Set 1 : rebased #Patch Set 2 : rebased #
Total comments: 17
Patch Set 3 : Address review comments #Patch Set 4 : Fix typo in comment #Patch Set 5 : rebased #Patch Set 6 : rebased #Patch Set 7 : Address review comments, rebase #Patch Set 8 : rebased #
Total comments: 22
Patch Set 9 : Address review comments #Patch Set 10 : rebased #Patch Set 11 : Use PostDelayedTaskWithTraits #Messages
Total messages: 98 (80 generated)
The CQ bit was checked by rogerta@chromium.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.chromium.or...
Description was changed from ========== Remove call to GetBlockPool. BUG=667892 ========== to ========== Remove call to GetBlockPool in RLZ. BUG=667892 ==========
Description was changed from ========== Remove call to GetBlockPool in RLZ. BUG=667892 ========== to ========== Remove call to GetBlockingPool in RLZ. BUG=667892 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rogerta@chromium.org changed reviewers: + gab@chromium.org
Hi Gab, Let me know if this looks correct. Thanks.
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for working on this, comments below. I don't think it works as is, but tests pass..?! https://codereview.chromium.org/2949263003/diff/160001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/160001/components/rlz/rlz_tra... components/rlz/rlz_tracker.cc:174: base::WithBaseSyncPrimitives()})) {} base::TaskPriority::BACKGROUND? (i.e. RLZ is a server notification with no influence on the user experience?) https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (left): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:310: // Run a blocking event loop to match the win inet implementation. Should we keep the comment to say that this is "blocking to match the win inet implementation"? https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:230: : event_(event) {} std::move(event) https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { Does this work? url_fetch.h says: // You may create the URLFetcher instance on any sequence; OnURLFetchComplete() // will be called back on the same sequence you use to create the instance. which tells me the blocking wait will prevent this sequence from processing this notification and signal as the URLFetcher is created on the stack just before the Wait(). What you could do to keep the API synchronous (blocking until done) while having a responsive sequence for the URLFetcher to notify is to create a dedicated sequence to create and start the URL fetcher. That way you'd block your calling sequence and wait for a signal from the processing sequence. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:369: base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start, net::URLFetcher::Start() seems to just PostTask itself: https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... so I was going to say that this call probably still needed to happen on SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking wait) but now I'm thinking you can probably just call it inline here.
The CQ bit was checked by rogerta@chromium.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.chromium.or...
The CQ bit was checked by rogerta@chromium.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.chromium.or...
Why don't you think this works? All tests pass, I built the code with rlz enabled and ran all the tests. I also manually tested that financial pings were sent out as expected, no deadlock as you describe. https://codereview.chromium.org/2949263003/diff/160001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/160001/components/rlz/rlz_tra... components/rlz/rlz_tracker.cc:174: base::WithBaseSyncPrimitives()})) {} On 2017/07/17 16:44:20, gab wrote: > base::TaskPriority::BACKGROUND? (i.e. RLZ is a server notification with no > influence on the user experience?) Done. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (left): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:310: // Run a blocking event loop to match the win inet implementation. On 2017/07/17 16:44:20, gab wrote: > Should we keep the comment to say that this is "blocking to match the win inet > implementation"? Done. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:230: : event_(event) {} On 2017/07/17 16:44:20, gab wrote: > std::move(event) This happens exactly once per run of chrome.exe in a background thread. Not sure the optimization is worth it. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2017/07/17 16:44:20, gab wrote: > Does this work? url_fetch.h says: > > // You may create the URLFetcher instance on any sequence; OnURLFetchComplete() > // will be called back on the same sequence you use to create the instance. > > which tells me the blocking wait will prevent this sequence from processing this > notification and signal as the URLFetcher is created on the stack just before > the Wait(). > > What you could do to keep the API synchronous (blocking until done) while having > a responsive sequence for the URLFetcher to notify is to create a dedicated > sequence to create and start the URL fetcher. > > That way you'd block your calling sequence and wait for a signal from the > processing sequence. I think that comment in URLFetcher may no longer be completely accurate. It seems to date from the days when we had dedicated threads. The behaviour as I experience it now is that the sequence that calls Start() is the one that will be used to run delegate. Since Start is being posted to another sequence (see line 369 below) there is no deadlock. Also why your comment at line 369 about calling Start() inline won't work. I could add a comment to clarify this. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:369: base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start, On 2017/07/17 16:44:20, gab wrote: > net::URLFetcher::Start() seems to just PostTask itself: > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... > > so I was going to say that this call probably still needed to happen on > SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking wait) > but now I'm thinking you can probably just call it inline here. No, see comment above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:230: : event_(event) {} On 2017/07/17 19:19:37, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/17 16:44:20, gab wrote: > > std::move(event) > > This happens exactly once per run of chrome.exe in a background thread. Not > sure the optimization is worth it. It's just a good habit to have when taking scoped_refptr (or any moveable type for that matter) by value as a parameter. Don't see why diverge here. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/17 16:44:20, gab wrote: > > Does this work? url_fetch.h says: > > > > // You may create the URLFetcher instance on any sequence; > OnURLFetchComplete() > > // will be called back on the same sequence you use to create the instance. > > > > which tells me the blocking wait will prevent this sequence from processing > this > > notification and signal as the URLFetcher is created on the stack just before > > the Wait(). > > > > What you could do to keep the API synchronous (blocking until done) while > having > > a responsive sequence for the URLFetcher to notify is to create a dedicated > > sequence to create and start the URL fetcher. > > > > That way you'd block your calling sequence and wait for a signal from the > > processing sequence. > > I think that comment in URLFetcher may no longer be completely accurate. It > seems to date from the days when we had dedicated threads. The behaviour as I > experience it now is that the sequence that calls Start() is the one that will > be used to run delegate. Since Start is being posted to another sequence (see > line 369 below) there is no deadlock. Also why your comment at line 369 about > calling Start() inline won't work. I could add a comment to clarify this. Hmmmm, well it's definitely also not running on the sequence of base::PostTask() because parallel tasks currently don't provide a sequence (i.e. SequencedTaskRunnerHandle::Get() returns null) -- we're planning to fix that to allow parallel tasks to be extended into a sequence should the task wish to grab a reference to itself for continuations but that's WIP. It seems it's running on URLRequestContextGetter::GetNetworkTaskRunner() which I'm having trouble figuring out where that goes in this case. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:369: base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start, On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/17 16:44:20, gab wrote: > > net::URLFetcher::Start() seems to just PostTask itself: > > > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... > > > > so I was going to say that this call probably still needed to happen on > > SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking > wait) > > but now I'm thinking you can probably just call it inline here. > > No, see comment above. If the fetcher replies on another sequence, isn't the |fetcher->| usage below racy in the event of a timeout which coincides with a late OnURLFetchComplete() (and already was prior to this CL)?
Description was changed from ========== Remove call to GetBlockingPool in RLZ. BUG=667892 ========== to ========== Remove call to GetBlockingPool in RLZ. BUG=667892, 618530 ==========
The CQ bit was checked by rogerta@chromium.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.chromium.or...
Thanks Gab, see below. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2017/07/17 21:05:20, gab wrote: > On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > > On 2017/07/17 16:44:20, gab wrote: > > > Does this work? url_fetch.h says: > > > > > > // You may create the URLFetcher instance on any sequence; > > OnURLFetchComplete() > > > // will be called back on the same sequence you use to create the instance. > > > > > > which tells me the blocking wait will prevent this sequence from processing > > this > > > notification and signal as the URLFetcher is created on the stack just > before > > > the Wait(). > > > > > > What you could do to keep the API synchronous (blocking until done) while > > having > > > a responsive sequence for the URLFetcher to notify is to create a dedicated > > > sequence to create and start the URL fetcher. > > > > > > That way you'd block your calling sequence and wait for a signal from the > > > processing sequence. > > > > I think that comment in URLFetcher may no longer be completely accurate. It > > seems to date from the days when we had dedicated threads. The behaviour as I > > experience it now is that the sequence that calls Start() is the one that will > > be used to run delegate. Since Start is being posted to another sequence (see > > line 369 below) there is no deadlock. Also why your comment at line 369 about > > calling Start() inline won't work. I could add a comment to clarify this. > > Hmmmm, well it's definitely also not running on the sequence of base::PostTask() > because parallel tasks currently don't provide a sequence (i.e. > SequencedTaskRunnerHandle::Get() returns null) -- we're planning to fix that to > allow parallel tasks to be extended into a sequence should the task wish to grab > a reference to itself for continuations but that's WIP. > > It seems it's running on URLRequestContextGetter::GetNetworkTaskRunner() which > I'm having trouble figuring out where that goes in this case. Got it. The main point is to not block this sequence, which is what happens here. The real requirements is for the delegate to run in "some other sequence" but it does not matter which one. If the other sequence changes in the future then all is still fine. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:369: base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start, On 2017/07/17 21:05:20, gab wrote: > On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > > On 2017/07/17 16:44:20, gab wrote: > > > net::URLFetcher::Start() seems to just PostTask itself: > > > > > > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... > > > > > > so I was going to say that this call probably still needed to happen on > > > SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking > > wait) > > > but now I'm thinking you can probably just call it inline here. > > > > No, see comment above. > > If the fetcher replies on another sequence, isn't the |fetcher->| usage below > racy in the event of a timeout which coincides with a late OnURLFetchComplete() > (and already was prior to this CL)? Correct, this was racy and still is. That's OK though, this is best effort only. Which is why it is done on a background thread with skip-on-shutdown semantics. If it does not get done this time it will happen on the next chrome run. If it happens this time and does not get "recorded", it will happen again next time but will be de-duped server side. By "not recorded" I mean the response is not parsed and remembered locally that it was done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:232: void OnURLFetchComplete(const net::URLFetcher* source) override { On 2017/07/21 13:53:42, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/17 21:05:20, gab wrote: > > On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > > > On 2017/07/17 16:44:20, gab wrote: > > > > Does this work? url_fetch.h says: > > > > > > > > // You may create the URLFetcher instance on any sequence; > > > OnURLFetchComplete() > > > > // will be called back on the same sequence you use to create the > instance. > > > > > > > > which tells me the blocking wait will prevent this sequence from > processing > > > this > > > > notification and signal as the URLFetcher is created on the stack just > > before > > > > the Wait(). > > > > > > > > What you could do to keep the API synchronous (blocking until done) while > > > having > > > > a responsive sequence for the URLFetcher to notify is to create a > dedicated > > > > sequence to create and start the URL fetcher. > > > > > > > > That way you'd block your calling sequence and wait for a signal from the > > > > processing sequence. > > > > > > I think that comment in URLFetcher may no longer be completely accurate. It > > > seems to date from the days when we had dedicated threads. The behaviour as > I > > > experience it now is that the sequence that calls Start() is the one that > will > > > be used to run delegate. Since Start is being posted to another sequence > (see > > > line 369 below) there is no deadlock. Also why your comment at line 369 > about > > > calling Start() inline won't work. I could add a comment to clarify this. > > > > Hmmmm, well it's definitely also not running on the sequence of > base::PostTask() > > because parallel tasks currently don't provide a sequence (i.e. > > SequencedTaskRunnerHandle::Get() returns null) -- we're planning to fix that > to > > allow parallel tasks to be extended into a sequence should the task wish to > grab > > a reference to itself for continuations but that's WIP. > > > > It seems it's running on URLRequestContextGetter::GetNetworkTaskRunner() which > > I'm having trouble figuring out where that goes in this case. > > Got it. The main point is to not block this sequence, which is what happens > here. The real requirements is for the delegate to run in "some other sequence" > but it does not matter which one. If the other sequence changes in the future > then all is still fine. True, so long as that doesn't become SequencedTaskRunnerHandle::Get() as then you'd hang (and since RLZ isn't really tested -- at least not on Chromium waterfall -- this can result in disasters, right?). I'm mostly leery because the documentation says explicitly that it happens on the sequence the URLFetcher is created on... so that doesn't make this feel very future proof... Can you check with the owners how this API's documentation should be updated to both be correct and future proof? If you prefer to avoid that, I think the most correct approach is to post the core of PingServer() on a different sequence (can be an inline base::CreateSequencedTaskRunnerWithTraits()->PostTask()). So that PingServer() essentially just becomes a Post+Wait. Then at least we're guaranteed that the Wait() will never be blocking the very signal it's waiting on. https://codereview.chromium.org/2949263003/diff/160001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:369: base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start, On 2017/07/21 13:53:42, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/17 21:05:20, gab wrote: > > On 2017/07/17 19:19:36, Roger Tawa OOO till Jul 10th wrote: > > > On 2017/07/17 16:44:20, gab wrote: > > > > net::URLFetcher::Start() seems to just PostTask itself: > > > > > > > > > > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... > > > > > > > > so I was going to say that this call probably still needed to happen on > > > > SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking > > > wait) > > > > but now I'm thinking you can probably just call it inline here. > > > > > > No, see comment above. > > > > If the fetcher replies on another sequence, isn't the |fetcher->| usage below > > racy in the event of a timeout which coincides with a late > OnURLFetchComplete() > > (and already was prior to this CL)? > > Correct, this was racy and still is. That's OK though, this is best effort > only. Which is why it is done on a background thread with skip-on-shutdown > semantics. > > If it does not get done this time it will happen on the next chrome run. If it > happens this time and does not get "recorded", it will happen again next time > but will be de-duped server side. By "not recorded" I mean the response is not > parsed and remembered locally that it was done. It's okay that this may fail and will be retried but what I'm saying is the code can result in a data race and this is never okay as it results in undefined behaviour : https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...
The CQ bit was checked by rogerta@chromium.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.chromium.or...
Patchset #6 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@rogerta: did my previous reply make sense to you? Happy to discuss further if not, thanks!
The CQ bit was checked by rogerta@chromium.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.chromium.or...
Hi Gab, Used CreateSequencedTaskRunnerWithTraits() as you suggested to make a new sequence. This required a little bit of refactoring, and now there is an int and a string passed between sequences. I added some synchronization to prevent races. Please take another look. Thanks.
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 rogerta@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ comments, this is great, thanks! https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... components/rlz/rlz_tracker.cc:519: void RLZTracker::ClearRlzStateImpl() { DCHECK(impl_task_runner_->RunsTasksOnCurrentSequence()); here and elsewhere to help document what runs where. https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.h (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... components/rlz/rlz_tracker.h:235: scoped_refptr<base::SequencedTaskRunner> task_runner_; More specific name (background_task_runner_? or impl_task_runner_?) https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:223: void SignalFetchComplete(int response_code, std::string&& response) { I'm all for r-value params but style guide still bans them in member methods: https://google.github.io/styleguide/cppguide.html#Rvalue_references https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:230: bool TimedWait(const base::TimeDelta& timeout) { Pass TimeDelta by value (see time.h) https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:239: void TakeResponse(std::string* response) { I think std::string TakeResponse(); is a simpler API, you can return std::move(response_) to get the same effect. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:242: response_.clear(); The above std::move() would clear() implicitly I assume. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:252: int response_code_; For POD types I prefer C++11 inline member initialization instead of initializer list, i.e. int response_code_ = net::URLFetcher::RESPONSE_CODE_INVALID; easier to read (and make sure a future member doesn't forget to init) https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:360: #if defined(RLZ_NETWORK_IMPLEMENTATION_WIN_INET) Curious, is this still ever a thing or should that be cleaned up (not in this CL of course) https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:419: base::PostTask(FROM_HERE, base::Bind(&ShutdownCheck, event)); PostTaskWithTraits with TaskPriority::BACKGROUND? https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:421: scoped_refptr<base::SequencedTaskRunner> background_runner( Add a comment here (or on PingRlzServer() or both) to explain why it's important to run this on a separate sequence as we've determined.
Thanks, all comments addressed. Will commit after rebase. https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.cc (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... components/rlz/rlz_tracker.cc:519: void RLZTracker::ClearRlzStateImpl() { On 2017/07/27 18:29:37, gab wrote: > DCHECK(impl_task_runner_->RunsTasksOnCurrentSequence()); > > here and elsewhere to help document what runs where. Added where it makes sense. https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... File components/rlz/rlz_tracker.h (right): https://codereview.chromium.org/2949263003/diff/300001/components/rlz/rlz_tra... components/rlz/rlz_tracker.h:235: scoped_refptr<base::SequencedTaskRunner> task_runner_; On 2017/07/27 18:29:37, gab wrote: > More specific name (background_task_runner_? or impl_task_runner_?) Done. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:223: void SignalFetchComplete(int response_code, std::string&& response) { On 2017/07/27 18:29:38, gab wrote: > I'm all for r-value params but style guide still bans them in member methods: > https://google.github.io/styleguide/cppguide.html#Rvalue_references Thanks, I had not seen that style guideline. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:230: bool TimedWait(const base::TimeDelta& timeout) { On 2017/07/27 18:29:37, gab wrote: > Pass TimeDelta by value (see time.h) Done. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:239: void TakeResponse(std::string* response) { On 2017/07/27 18:29:38, gab wrote: > I think > > std::string TakeResponse(); > > is a simpler API, you can > > return std::move(response_) to get the same effect. Done. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:242: response_.clear(); On 2017/07/27 18:29:37, gab wrote: > The above std::move() would clear() implicitly I assume. Actually, it does not. After an object is moved, it is left in an unspecified but consistent state. I want to clear() it so that its value becomes specified (i.e. empty). https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:252: int response_code_; On 2017/07/27 18:29:38, gab wrote: > For POD types I prefer C++11 inline member initialization instead of initializer > list, i.e. > > int response_code_ = net::URLFetcher::RESPONSE_CODE_INVALID; > > easier to read (and make sure a future member doesn't forget to init) Done. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:360: #if defined(RLZ_NETWORK_IMPLEMENTATION_WIN_INET) On 2017/07/27 18:29:38, gab wrote: > Curious, is this still ever a thing or should that be cleaned up (not in this CL > of course) Possibly, not sure, but not in this CL as you say. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:419: base::PostTask(FROM_HERE, base::Bind(&ShutdownCheck, event)); On 2017/07/27 18:29:38, gab wrote: > PostTaskWithTraits with TaskPriority::BACKGROUND? Done. https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:421: scoped_refptr<base::SequencedTaskRunner> background_runner( On 2017/07/27 18:29:38, gab wrote: > Add a comment here (or on PingRlzServer() or both) to explain why it's important > to run this on a separate sequence as we've determined. Done.
The CQ bit was checked by rogerta@chromium.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.chromium.or...
I forgot to respond to one comment earlier. While RLZ is not enabled in chrome.exe except for official builds, all unit tests and browsers are compiled and run on chromium builds: https://cs.chromium.org/chromium/src/rlz/features/features.gni RLZ has a lot of tests and they are run regularly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:242: response_.clear(); On 2017/07/28 18:43:06, Roger Tawa OOO till Jul 10th wrote: > On 2017/07/27 18:29:37, gab wrote: > > The above std::move() would clear() implicitly I assume. > > Actually, it does not. After an object is moved, it is left in an unspecified > but consistent state. I want to clear() it so that its value becomes specified > (i.e. empty). Hmmm true, but you only TakeResponse() once per RefCountedWaitableEvent so I don't see why it matters? Can't TakeResponse() also just have move semantics (leaving |response_| in a valid but unspecified state)?
https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... rlz/lib/financial_ping.cc:242: response_.clear(); On 2017/07/28 19:28:57, gab wrote: > On 2017/07/28 18:43:06, Roger Tawa OOO till Jul 10th wrote: > > On 2017/07/27 18:29:37, gab wrote: > > > The above std::move() would clear() implicitly I assume. > > > > Actually, it does not. After an object is moved, it is left in an unspecified > > but consistent state. I want to clear() it so that its value becomes > specified > > (i.e. empty). > > Hmmm true, but you only TakeResponse() once per RefCountedWaitableEvent so I > don't see why it matters? Can't TakeResponse() also just have move semantics > (leaving |response_| in a valid but unspecified state)? I'd rather not leave the member variable in an unspecified state.
The CQ bit was checked by rogerta@chromium.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.chromium.or...
rogerta@chromium.org changed reviewers: + droger@chromium.org
Salut David, Can you do an owner review for: ios/chrome/browser/rlz/rlz_tracker_delegate_impl.h|cc Thanks.
On 2017/07/28 19:42:17, Roger Tawa OOO till Jul 10th wrote: > https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping.cc > File rlz/lib/financial_ping.cc (right): > > https://codereview.chromium.org/2949263003/diff/300001/rlz/lib/financial_ping... > rlz/lib/financial_ping.cc:242: response_.clear(); > On 2017/07/28 19:28:57, gab wrote: > > On 2017/07/28 18:43:06, Roger Tawa OOO till Jul 10th wrote: > > > On 2017/07/27 18:29:37, gab wrote: > > > > The above std::move() would clear() implicitly I assume. > > > > > > Actually, it does not. After an object is moved, it is left in an > unspecified > > > but consistent state. I want to clear() it so that its value becomes > > specified > > > (i.e. empty). > > > > Hmmm true, but you only TakeResponse() once per RefCountedWaitableEvent so I > > don't see why it matters? Can't TakeResponse() also just have move semantics > > (leaving |response_| in a valid but unspecified state)? > > I'd rather not leave the member variable in an unspecified state. The reason std::move leaves it in an unspecified state is to avoid doing work for something that won't be re-used. Unspecified doesn't mean undefined. The state is guaranteed to be valid, but it could say decide to leave the old string there if it's a short one allocated on the stack and is copied out instead of one on the heap which is simply transferred. I don't see the point of explicitly clearing a value you're never going to re-use. Anyways, details in this case, but my 2c.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rogerta@chromium.org changed reviewers: + rohitrao@chromium.org
ios/ lgtm
The CQ bit was checked by rogerta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2949263003/#ps360001 (title: "Use PostDelayedTaskWithTraits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1501598918271700, "parent_rev": "dbced2bcec41b3567f120091e243169bb276ca93", "commit_rev": "c4a9c77f6e6c59b02fbb09db8da441f630b9ac54"}
Message was sent while issue was closed.
Description was changed from ========== Remove call to GetBlockingPool in RLZ. BUG=667892, 618530 ========== to ========== Remove call to GetBlockingPool in RLZ. BUG=667892, 618530 Review-Url: https://codereview.chromium.org/2949263003 Cr-Commit-Position: refs/heads/master@{#491013} Committed: https://chromium.googlesource.com/chromium/src/+/c4a9c77f6e6c59b02fbb09db8da4... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:360001) as https://chromium.googlesource.com/chromium/src/+/c4a9c77f6e6c59b02fbb09db8da4... |