Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893943002:
[NetworkChangeNotifier] Run Windows connection type computation on other thread (Closed)
|
Created:
3 years, 7 months ago by jkarlin Modified:
3 years, 7 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[NetworkChangeNotifier] Run Windows connection type computation on safe thread
1. Move RecomputeCurrentConnectionType to anonymous namespace
2. Call RecomputeCurrentConnectionType via callback so tests can override
3. Call RecomputeCurrentConnectionType on dns thread
4. Tests inject their own dns task runner
BUG=721461, 725487
Review-Url: https://codereview.chromium.org/2893943002
Cr-Commit-Position: refs/heads/master@{#475082}
Committed: https://chromium.googlesource.com/chromium/src/+/6b0ea35f8c93c6eb229ba09624d566a339b3073b
Patch Set 1 #Patch Set 2 : Nit #
Total comments: 9
Patch Set 3 : Address comments from PS2 #
Total comments: 2
Patch Set 4 : Address comments from PS3 #
Total comments: 7
Patch Set 5 : Virtual #Patch Set 6 : Works #Patch Set 7 : Nits #
Total comments: 2
Messages
Total messages: 51 (37 generated)
The CQ bit was checked by jkarlin@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 ========== [NetworkChangeNotifier] Run Windows connection type computation on other thread BUG= ========== to ========== [NetworkChangeNotifier] Run Windows connection type computation on other thread BUG=721461 ==========
Description was changed from ========== [NetworkChangeNotifier] Run Windows connection type computation on other thread BUG=721461 ========== to ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread so that any registry functions run on a thread meant for disk operations. 4. Tests inject their own dns task runner. BUG=721461 ==========
Description was changed from ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread so that any registry functions run on a thread meant for disk operations. 4. Tests inject their own dns task runner. BUG=721461 ========== to ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread 4. Tests inject their own dns task runner BUG=721461 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jkarlin@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...
jkarlin@chromium.org changed reviewers: + pauljensen@chromium.org
Paul, PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier.h:363: static ConnectionType ConnectionTypeFromInterfaces(); Can we move this protected again? You can make RecomputeCurrentConnectionType() a NCNWin static member. https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.cc:350: ConnectionType connection_type2 = CONNECTION_NONE; can we give this variable a more descriptive name? https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.h:105: scoped_refptr<base::SingleThreadTaskRunner> dns_task_runner_; Hmm, can we avoid dns_task_runner_ and calculate_connection_type_ by instead moving the DnsConfigServiceThread decl into this file, then having the test reset dns_config_service_thread_ to be some overloaded impl in tests?
Thanks! PTAL. https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier.h:363: static ConnectionType ConnectionTypeFromInterfaces(); On 2017/05/22 18:01:51, pauljensen wrote: > Can we move this protected again? You can make RecomputeCurrentConnectionType() > a NCNWin static member. Done. https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.cc:350: ConnectionType connection_type2 = CONNECTION_NONE; On 2017/05/22 18:01:51, pauljensen wrote: > can we give this variable a more descriptive name? Done. https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.h:105: scoped_refptr<base::SingleThreadTaskRunner> dns_task_runner_; On 2017/05/22 18:01:51, pauljensen wrote: > Hmm, can we avoid dns_task_runner_ and calculate_connection_type_ by instead > moving the DnsConfigServiceThread decl into this file, then having the test > reset dns_config_service_thread_ to be some overloaded impl in tests? We could get rid of dns_task_runner_ that way I suppose, but injecting task runners is pretty standard procedure and in this case seems pretty reasonable. We could get rid of calculate_connection_type_ if we made it a virtual member function of DnsConfigServiceThread and made DnsConfigServiceThread ref counted. Would that be preferable? I'm not so sure as it changes the meaning of the class.
The CQ bit was checked by jkarlin@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.
Description was changed from ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread 4. Tests inject their own dns task runner BUG=721461 ========== to ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread 4. Tests inject their own dns task runner BUG=721461,725487 ==========
https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.h:105: scoped_refptr<base::SingleThreadTaskRunner> dns_task_runner_; On 2017/05/23 11:44:38, jkarlin wrote: > On 2017/05/22 18:01:51, pauljensen wrote: > > Hmm, can we avoid dns_task_runner_ and calculate_connection_type_ by instead > > moving the DnsConfigServiceThread decl into this file, then having the test > > reset dns_config_service_thread_ to be some overloaded impl in tests? > > We could get rid of dns_task_runner_ that way I suppose, but injecting task > runners is pretty standard procedure and in this case seems pretty reasonable. > > We could get rid of calculate_connection_type_ if we made it a virtual member > function of DnsConfigServiceThread and made DnsConfigServiceThread ref counted. > Would that be preferable? I'm not so sure as it changes the meaning of the > class. Why would DnsConfigServiceThread need to be ref counted? It's owned by NCNWin. https://codereview.chromium.org/2893943002/diff/160001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/160001/net/base/network_chang... net/base/network_change_notifier_win.cc:96: } can we move this back down?
The CQ bit was checked by jkarlin@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, PTAL. https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win.h:105: scoped_refptr<base::SingleThreadTaskRunner> dns_task_runner_; On 2017/05/23 14:55:50, pauljensen wrote: > On 2017/05/23 11:44:38, jkarlin wrote: > > On 2017/05/22 18:01:51, pauljensen wrote: > > > Hmm, can we avoid dns_task_runner_ and calculate_connection_type_ by instead > > > moving the DnsConfigServiceThread decl into this file, then having the test > > > reset dns_config_service_thread_ to be some overloaded impl in tests? > > > > We could get rid of dns_task_runner_ that way I suppose, but injecting task > > runners is pretty standard procedure and in this case seems pretty reasonable. > > > > We could get rid of calculate_connection_type_ if we made it a virtual member > > function of DnsConfigServiceThread and made DnsConfigServiceThread ref > counted. > > Would that be preferable? I'm not so sure as it changes the meaning of the > > class. > > Why would DnsConfigServiceThread need to be ref counted? It's owned by NCNWin. Ah, you're right. All tasks on the dns thread will finish running (thanks to the call to Stop()) before the dns thread is destroyed, so it's safe. Can be done, but I don't see a strong reason to prefer that over what's in the CL. https://codereview.chromium.org/2893943002/diff/160001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/160001/net/base/network_chang... net/base/network_change_notifier_win.cc:96: } On 2017/05/23 14:55:51, pauljensen wrote: > can we move this back down? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... File net/base/network_change_notifier_win_unittest.cc (right): https://codereview.chromium.org/2893943002/diff/140001/net/base/network_chang... net/base/network_change_notifier_win_unittest.cc:60: dns_task_runner_ = base::ThreadTaskRunnerHandle::Get(); why do we care what thread this is run on in tests? we're already spinning the message loop on line 205, do we need to also spin the DNS loop? https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.cc:233: weak_factory_.GetWeakPtr())); do we need to use a weakptr if NCNWin is guaranteed to outlive dns thread? https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.h:70: static ConnectionType RecomputeCurrentConnectionType(); can we leave this virtual since NCNWin outlives dns thread? then tests can overload it, and we can get rid of calculate_connection_type_
https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.cc (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.cc:233: weak_factory_.GetWeakPtr())); On 2017/05/23 17:57:40, pauljensen wrote: > do we need to use a weakptr if NCNWin is guaranteed to outlive dns thread? see previous response. https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.h:70: static ConnectionType RecomputeCurrentConnectionType(); On 2017/05/23 17:57:40, pauljensen wrote: > can we leave this virtual since NCNWin outlives dns thread? then tests can > overload it, and we can get rid of calculate_connection_type_ NCNWin outlives dns thread, but there is no guarantee that the NetworkChangeNotifierWin will still be alive by the time the reply task is run. E.g., 1. On main thread NCN PostTaskAndReply's to dns thread 2. dns thread does its thing and posts a task on the main thread 3. main thread runs a task to delete the NCN 4. main thread runs the reply task, but the NCN is gone
https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.h:70: static ConnectionType RecomputeCurrentConnectionType(); On 2017/05/23 18:02:15, jkarlin wrote: > On 2017/05/23 17:57:40, pauljensen wrote: > > can we leave this virtual since NCNWin outlives dns thread? then tests can > > overload it, and we can get rid of calculate_connection_type_ > > NCNWin outlives dns thread, but there is no guarantee that the > NetworkChangeNotifierWin will still be alive by the time the reply task is run. > > E.g., > > 1. On main thread NCN PostTaskAndReply's to dns thread > 2. dns thread does its thing and posts a task on the main thread > 3. main thread runs a task to delete the NCN > 4. main thread runs the reply task, but the NCN is gone can it post back with a weakptr?
https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.h:70: static ConnectionType RecomputeCurrentConnectionType(); On 2017/05/23 18:05:47, pauljensen wrote: > On 2017/05/23 18:02:15, jkarlin wrote: > > On 2017/05/23 17:57:40, pauljensen wrote: > > > can we leave this virtual since NCNWin outlives dns thread? then tests can > > > overload it, and we can get rid of calculate_connection_type_ > > > > NCNWin outlives dns thread, but there is no guarantee that the > > NetworkChangeNotifierWin will still be alive by the time the reply task is > run. > > > > E.g., > > > > 1. On main thread NCN PostTaskAndReply's to dns thread > > 2. dns thread does its thing and posts a task on the main thread > > 3. main thread runs a task to delete the NCN > > 4. main thread runs the reply task, but the NCN is gone > > can it post back with a weakptr? I think this is very similar to the example here: https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...
The CQ bit was checked by jkarlin@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.
The CQ bit was checked by jkarlin@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...
After F2F discussion, changed the CL to instead use a virtual method for the PostTaskAndReply which tests can override. PTAL, thanks! https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/180001/net/base/network_chang... net/base/network_change_notifier_win.h:70: static ConnectionType RecomputeCurrentConnectionType(); On 2017/05/23 18:06:47, pauljensen wrote: > On 2017/05/23 18:05:47, pauljensen wrote: > > On 2017/05/23 18:02:15, jkarlin wrote: > > > On 2017/05/23 17:57:40, pauljensen wrote: > > > > can we leave this virtual since NCNWin outlives dns thread? then tests can > > > > overload it, and we can get rid of calculate_connection_type_ > > > > > > NCNWin outlives dns thread, but there is no guarantee that the > > > NetworkChangeNotifierWin will still be alive by the time the reply task is > > run. > > > > > > E.g., > > > > > > 1. On main thread NCN PostTaskAndReply's to dns thread > > > 2. dns thread does its thing and posts a task on the main thread > > > 3. main thread runs a task to delete the NCN > > > 4. main thread runs the reply task, but the NCN is gone > > > > can it post back with a weakptr? > > I think this is very similar to the example here: > https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta... Back to the original.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one optional comment https://codereview.chromium.org/2893943002/diff/240001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/240001/net/base/network_chang... net/base/network_change_notifier_win.h:69: base::Callback<void(ConnectionType)> reply_callback) const; Instead of having every caller assemble the Callback by Binding the method to a weakptr, you could make this function take a C++ pointer-to-member-function and just have one Bind call inside this function. It might simplify the code slightly. This is one of the very rare instances where C++ pointer-to-member-function is useful...it'd be a pity to pass it up :) But it's up to you.
The CQ bit was checked by jkarlin@chromium.org
Thanks! https://codereview.chromium.org/2893943002/diff/240001/net/base/network_chang... File net/base/network_change_notifier_win.h (right): https://codereview.chromium.org/2893943002/diff/240001/net/base/network_chang... net/base/network_change_notifier_win.h:69: base::Callback<void(ConnectionType)> reply_callback) const; On 2017/05/26 18:29:40, pauljensen wrote: > Instead of having every caller assemble the Callback by Binding the method to a > weakptr, you could make this function take a C++ pointer-to-member-function and > just have one Bind call inside this function. It might simplify the code > slightly. This is one of the very rare instances where C++ > pointer-to-member-function is useful...it'd be a pity to pass it up :) But it's > up to you. Passing callbacks is more general however. I'd rather stick with that.
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": 240001, "attempt_start_ts": 1495825413961200,
"parent_rev": "33fcecd6c91552f374b30b153d7a914bd74d52e8", "commit_rev":
"cd2c7ff2ab6277daed0b3ffaa951ad11da2ab8c1"}
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495825413961200,
"parent_rev": "b1b08ceaa01327a6ba395855d578225a752004fb", "commit_rev":
"6b0ea35f8c93c6eb229ba09624d566a339b3073b"}
Message was sent while issue was closed.
Description was changed from ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread 4. Tests inject their own dns task runner BUG=721461,725487 ========== to ========== [NetworkChangeNotifier] Run Windows connection type computation on safe thread 1. Move RecomputeCurrentConnectionType to anonymous namespace 2. Call RecomputeCurrentConnectionType via callback so tests can override 3. Call RecomputeCurrentConnectionType on dns thread 4. Tests inject their own dns task runner BUG=721461,725487 Review-Url: https://codereview.chromium.org/2893943002 Cr-Commit-Position: refs/heads/master@{#475082} Committed: https://chromium.googlesource.com/chromium/src/+/6b0ea35f8c93c6eb229ba09624d5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001) as https://chromium.googlesource.com/chromium/src/+/6b0ea35f8c93c6eb229ba09624d5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
