Merge lp:~thomas-voss/location-service/fix-nm-race into lp:location-service/trunk
- fix-nm-race
- Merge into trunk
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Thomas Voß | ||||||||
Approved revision: | 153 | ||||||||
Merged at revision: | 148 | ||||||||
Proposed branch: | lp:~thomas-voss/location-service/fix-nm-race | ||||||||
Merge into: | lp:location-service/trunk | ||||||||
Diff against target: |
219 lines (+102/-60) 3 files modified
src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp (+1/-2) src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp (+97/-56) src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h (+4/-2) |
||||||||
To merge this branch: | bzr merge lp:~thomas-voss/location-service/fix-nm-race | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Loïc Minier | Approve | ||
PS Jenkins bot | continuous-integration | Approve | |
Review via email: mp+241090@code.launchpad.net |
Commit message
Make sure that devices being added/removed by NetworkManager are handled correctly.
Description of the change
Make sure that devices being added/removed by NetworkManager are handled correctly.
- 148. By Thomas Voß
-
Lock the cache when accessing it from signal handlers.
PS Jenkins bot (ps-jenkins) wrote : | # |
Loïc Minier (lool) wrote : | # |
Looks good outside of not being sure about default connection handling
- 149. By Thomas Voß
-
Do not recalculate connection characteristics for every new device. Instead, rely on the established way of listening for changes to the active connection obect.
- 150. By Thomas Voß
-
Bail out if the cache already knows about the device.
- 151. By Thomas Voß
-
Add missing ;.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:149
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:151
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:149
http://
Executed test runs:
None: http://
None: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Loïc Minier (lool) wrote : | # |
(Latest version still breaks HERE)
- 152. By Thomas Voß
-
Disconnect from properties changed signal on destructin, as mentioned in the TODO.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:152
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 153. By Thomas Voß
-
Make sure that locking is done correctly.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:153
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Loïc Minier (lool) wrote : | # |
Hmm the locking is a bit spaghetti now; I can see why lock needs to be passed around to unlock before dispatching the updates from the on_foo() callbacks, but it does feel a bit weird to pass the lock down a callback and unlock it from there.
I guess the only way would be to queue the added/removed wifis/cells into a queue and trigger the updates once the lock is released, but that's pretty much as weird.
Anyway, tested, and seems to help with the otherwise failing test case.
Note that: a) I wasn't able to reproduce on mako + vivid, so couldn't confirm this fixed anything; b) it did help reproducible breakage on krillin from earlier revisions, but I did manage to break HERE on one boot.
So generally, this helps; I suspect there's still a race for a case.
I did witness tech=cell switching to tech=wifi on krillin when coming out of airplane mode, which was nice!
Thomas Voß (thomas-voss) wrote : | # |
> Hmm the locking is a bit spaghetti now; I can see why lock needs to be passed
> around to unlock before dispatching the updates from the on_foo() callbacks,
> but it does feel a bit weird to pass the lock down a callback and unlock it
> from there.
>
The one case is technically not a callback but a functor passed for evaluation. In all other cases, the lock is created within the callback and handed down to the actual function. I do prefer this API style to naming functions *_locked(...), which is tedious and error prone.
> I guess the only way would be to queue the added/removed wifis/cells into a
> queue and trigger the updates once the lock is released, but that's pretty
> much as weird.
>
> Anyway, tested, and seems to help with the otherwise failing test case.
>
> Note that: a) I wasn't able to reproduce on mako + vivid, so couldn't confirm
> this fixed anything; b) it did help reproducible breakage on krillin from
> earlier revisions, but I did manage to break HERE on one boot.
>
> So generally, this helps; I suspect there's still a race for a case.
>
> I did witness tech=cell switching to tech=wifi on krillin when coming out of
> airplane mode, which was nice!
Thanks.
Preview Diff
1 | === modified file 'src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp' | |||
2 | --- src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp 2014-10-13 15:20:56 +0000 | |||
3 | +++ src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp 2014-11-10 20:13:24 +0000 | |||
4 | @@ -172,8 +172,7 @@ | |||
5 | 172 | 172 | ||
6 | 173 | detail::CachedRadioCell::~CachedRadioCell() | 173 | detail::CachedRadioCell::~CachedRadioCell() |
7 | 174 | { | 174 | { |
10 | 175 | // TODO(tvoss): Reenable this once we know why the modem cache gets flushed. | 175 | modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed); |
9 | 176 | // modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed); | ||
11 | 177 | } | 176 | } |
12 | 178 | 177 | ||
13 | 179 | const core::Property<bool>& detail::CachedRadioCell::is_roaming() const | 178 | const core::Property<bool>& detail::CachedRadioCell::is_roaming() const |
14 | 180 | 179 | ||
15 | === modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp' | |||
16 | --- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2014-10-13 13:54:05 +0000 | |||
17 | +++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2014-11-10 20:13:24 +0000 | |||
18 | @@ -423,56 +423,8 @@ | |||
19 | 423 | 423 | ||
20 | 424 | network_manager->for_each_device([this](const core::dbus::types::ObjectPath& device_path) | 424 | network_manager->for_each_device([this](const core::dbus::types::ObjectPath& device_path) |
21 | 425 | { | 425 | { |
72 | 426 | auto device = network_manager->device_for_path(device_path); | 426 | std::unique_lock<std::mutex> ul{cached.guard}; |
73 | 427 | 427 | on_device_added(device_path, ul); | |
24 | 428 | if (device.type() == xdg::NetworkManager::Device::Type::wifi) | ||
25 | 429 | { | ||
26 | 430 | // Make the device known to the cache. | ||
27 | 431 | std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it; | ||
28 | 432 | std::tie(it, std::ignore) = cached.wireless_devices.insert(std::make_pair(device_path, device)); | ||
29 | 433 | |||
30 | 434 | // Iterate over all currently known wifis | ||
31 | 435 | it->second.for_each_access_point([this, device_path](const core::dbus::types::ObjectPath& path) | ||
32 | 436 | { | ||
33 | 437 | try | ||
34 | 438 | { | ||
35 | 439 | on_access_point_added(path, device_path); | ||
36 | 440 | } | ||
37 | 441 | catch (const std::exception& e) | ||
38 | 442 | { | ||
39 | 443 | VLOG(1) << "Error while creating ap/wifi: " << e.what(); | ||
40 | 444 | } | ||
41 | 445 | }); | ||
42 | 446 | |||
43 | 447 | it->second.signals.scan_done->connect([this]() | ||
44 | 448 | { | ||
45 | 449 | signals.wireless_network_scan_finished(); | ||
46 | 450 | }); | ||
47 | 451 | |||
48 | 452 | it->second.signals.ap_added->connect([this, device_path](const core::dbus::types::ObjectPath& path) | ||
49 | 453 | { | ||
50 | 454 | try | ||
51 | 455 | { | ||
52 | 456 | on_access_point_added(path, device_path); | ||
53 | 457 | } | ||
54 | 458 | catch (const std::exception& e) | ||
55 | 459 | { | ||
56 | 460 | VLOG(1) << "Error while creating ap/wifi: " << e.what(); | ||
57 | 461 | } | ||
58 | 462 | }); | ||
59 | 463 | |||
60 | 464 | it->second.signals.ap_removed->connect([this](const core::dbus::types::ObjectPath& path) | ||
61 | 465 | { | ||
62 | 466 | try | ||
63 | 467 | { | ||
64 | 468 | on_access_point_removed(path); | ||
65 | 469 | } | ||
66 | 470 | catch (const std::exception& e) | ||
67 | 471 | { | ||
68 | 472 | VLOG(1) << "Error while removing ap/wifi: " << e.what(); | ||
69 | 473 | } | ||
70 | 474 | }); | ||
71 | 475 | } | ||
74 | 476 | }); | 428 | }); |
75 | 477 | 429 | ||
76 | 478 | // Query the initial connectivity state | 430 | // Query the initial connectivity state |
77 | @@ -482,6 +434,28 @@ | |||
78 | 482 | characteristics_for_connection( | 434 | characteristics_for_connection( |
79 | 483 | network_manager->properties.primary_connection->get())); | 435 | network_manager->properties.primary_connection->get())); |
80 | 484 | 436 | ||
81 | 437 | network_manager->signals.device_added->connect([this](const core::dbus::types::ObjectPath& path) | ||
82 | 438 | { | ||
83 | 439 | // We dispatch determining the connection characteristics to unblock | ||
84 | 440 | // the bus here. | ||
85 | 441 | dispatcher.service.post([this, path]() | ||
86 | 442 | { | ||
87 | 443 | std::unique_lock<std::mutex> ul{cached.guard}; | ||
88 | 444 | on_device_added(path, ul); | ||
89 | 445 | }); | ||
90 | 446 | }); | ||
91 | 447 | |||
92 | 448 | network_manager->signals.device_removed->connect([this](const core::dbus::types::ObjectPath& path) | ||
93 | 449 | { | ||
94 | 450 | // We dispatch determining the connection characteristics to unblock | ||
95 | 451 | // the bus here. | ||
96 | 452 | dispatcher.service.post([this, path]() | ||
97 | 453 | { | ||
98 | 454 | std::unique_lock<std::mutex> ul{cached.guard}; | ||
99 | 455 | on_device_removed(path, ul); | ||
100 | 456 | }); | ||
101 | 457 | }); | ||
102 | 458 | |||
103 | 485 | // And we wire up to property changes here | 459 | // And we wire up to property changes here |
104 | 486 | network_manager->signals.properties_changed->connect([this](const std::map<std::string, core::dbus::types::Variant>& dict) | 460 | network_manager->signals.properties_changed->connect([this](const std::map<std::string, core::dbus::types::Variant>& dict) |
105 | 487 | { | 461 | { |
106 | @@ -546,12 +520,79 @@ | |||
107 | 546 | }); | 520 | }); |
108 | 547 | } | 521 | } |
109 | 548 | 522 | ||
110 | 523 | void connectivity::OfonoNmConnectivityManager::Private::on_device_added( | ||
111 | 524 | const core::dbus::types::ObjectPath& device_path, | ||
112 | 525 | std::unique_lock<std::mutex>& ul) | ||
113 | 526 | { | ||
114 | 527 | if (cached.wireless_devices.count(device_path) > 0) | ||
115 | 528 | return; | ||
116 | 529 | |||
117 | 530 | auto device = network_manager->device_for_path(device_path); | ||
118 | 531 | |||
119 | 532 | if (device.type() == xdg::NetworkManager::Device::Type::wifi) | ||
120 | 533 | { | ||
121 | 534 | // Make the device known to the cache. | ||
122 | 535 | std::map<core::dbus::types::ObjectPath, org::freedesktop::NetworkManager::Device>::iterator it; | ||
123 | 536 | std::tie(it, std::ignore) = cached.wireless_devices.insert(std::make_pair(device_path, device)); | ||
124 | 537 | |||
125 | 538 | // Iterate over all currently known wifis | ||
126 | 539 | it->second.for_each_access_point([this, device_path, &ul](const core::dbus::types::ObjectPath& path) | ||
127 | 540 | { | ||
128 | 541 | try | ||
129 | 542 | { | ||
130 | 543 | on_access_point_added(path, device_path, ul); | ||
131 | 544 | } | ||
132 | 545 | catch (const std::exception& e) | ||
133 | 546 | { | ||
134 | 547 | VLOG(1) << "Error while creating ap/wifi: " << e.what(); | ||
135 | 548 | } | ||
136 | 549 | }); | ||
137 | 550 | |||
138 | 551 | it->second.signals.scan_done->connect([this]() | ||
139 | 552 | { | ||
140 | 553 | signals.wireless_network_scan_finished(); | ||
141 | 554 | }); | ||
142 | 555 | |||
143 | 556 | it->second.signals.ap_added->connect([this, device_path](const core::dbus::types::ObjectPath& path) | ||
144 | 557 | { | ||
145 | 558 | try | ||
146 | 559 | { | ||
147 | 560 | std::unique_lock<std::mutex> ul{cached.guard}; | ||
148 | 561 | on_access_point_added(path, device_path, ul); | ||
149 | 562 | } | ||
150 | 563 | catch (const std::exception& e) | ||
151 | 564 | { | ||
152 | 565 | VLOG(1) << "Error while creating ap/wifi: " << e.what(); | ||
153 | 566 | } | ||
154 | 567 | }); | ||
155 | 568 | |||
156 | 569 | it->second.signals.ap_removed->connect([this](const core::dbus::types::ObjectPath& path) | ||
157 | 570 | { | ||
158 | 571 | try | ||
159 | 572 | { | ||
160 | 573 | std::unique_lock<std::mutex> ul{cached.guard}; | ||
161 | 574 | on_access_point_removed(path, ul); | ||
162 | 575 | } | ||
163 | 576 | catch (const std::exception& e) | ||
164 | 577 | { | ||
165 | 578 | VLOG(1) << "Error while removing ap/wifi: " << e.what(); | ||
166 | 579 | } | ||
167 | 580 | }); | ||
168 | 581 | } | ||
169 | 582 | } | ||
170 | 583 | |||
171 | 584 | void connectivity::OfonoNmConnectivityManager::Private::on_device_removed( | ||
172 | 585 | const core::dbus::types::ObjectPath& path, | ||
173 | 586 | std::unique_lock<std::mutex>&) | ||
174 | 587 | { | ||
175 | 588 | cached.wireless_devices.erase(path); | ||
176 | 589 | } | ||
177 | 590 | |||
178 | 549 | void connectivity::OfonoNmConnectivityManager::Private::on_access_point_added( | 591 | void connectivity::OfonoNmConnectivityManager::Private::on_access_point_added( |
179 | 550 | const core::dbus::types::ObjectPath& ap_path, | 592 | const core::dbus::types::ObjectPath& ap_path, |
181 | 551 | const core::dbus::types::ObjectPath& device_path) | 593 | const core::dbus::types::ObjectPath& device_path, |
182 | 594 | std::unique_lock<std::mutex>& ul) | ||
183 | 552 | { | 595 | { |
184 | 553 | std::unique_lock<std::mutex> ul(cached.guard); | ||
185 | 554 | |||
186 | 555 | // Let's see if we have a device known for the path. We return early | 596 | // Let's see if we have a device known for the path. We return early |
187 | 556 | // if we do not know about the device. | 597 | // if we do not know about the device. |
188 | 557 | auto itd = cached.wireless_devices.find(device_path); | 598 | auto itd = cached.wireless_devices.find(device_path); |
189 | @@ -571,10 +612,10 @@ | |||
190 | 571 | ul.unlock(); signals.wireless_network_added(wifi); | 612 | ul.unlock(); signals.wireless_network_added(wifi); |
191 | 572 | } | 613 | } |
192 | 573 | 614 | ||
194 | 574 | void connectivity::OfonoNmConnectivityManager::Private::on_access_point_removed(const core::dbus::types::ObjectPath& ap_path) | 615 | void connectivity::OfonoNmConnectivityManager::Private::on_access_point_removed( |
195 | 616 | const core::dbus::types::ObjectPath& ap_path, | ||
196 | 617 | std::unique_lock<std::mutex>& ul) | ||
197 | 575 | { | 618 | { |
198 | 576 | std::unique_lock<std::mutex> ul(cached.guard); | ||
199 | 577 | |||
200 | 578 | // Let's see if we know about the wifi. We return early if not. | 619 | // Let's see if we know about the wifi. We return early if not. |
201 | 579 | auto itw = cached.wifis.find(ap_path); | 620 | auto itw = cached.wifis.find(ap_path); |
202 | 580 | if (itw == cached.wifis.end()) | 621 | if (itw == cached.wifis.end()) |
203 | 581 | 622 | ||
204 | === modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h' | |||
205 | --- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h 2014-10-08 14:17:54 +0000 | |||
206 | +++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.h 2014-11-10 20:13:24 +0000 | |||
207 | @@ -94,8 +94,10 @@ | |||
208 | 94 | 94 | ||
209 | 95 | // All network stack specific functionality goes here. | 95 | // All network stack specific functionality goes here. |
210 | 96 | void setup_network_stack_access(); | 96 | void setup_network_stack_access(); |
213 | 97 | void on_access_point_added(const core::dbus::types::ObjectPath& ap_path, const core::dbus::types::ObjectPath& device_path); | 97 | void on_device_added(const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul); |
214 | 98 | void on_access_point_removed(const core::dbus::types::ObjectPath& ap_path); | 98 | void on_device_removed(const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul); |
215 | 99 | void on_access_point_added(const core::dbus::types::ObjectPath& ap_path, const core::dbus::types::ObjectPath& device_path, std::unique_lock<std::mutex>& ul); | ||
216 | 100 | void on_access_point_removed(const core::dbus::types::ObjectPath& ap_path, std::unique_lock<std::mutex>& ul); | ||
217 | 99 | com::ubuntu::location::connectivity::Characteristics characteristics_for_connection(const core::dbus::types::ObjectPath& path); | 101 | com::ubuntu::location::connectivity::Characteristics characteristics_for_connection(const core::dbus::types::ObjectPath& path); |
218 | 100 | 102 | ||
219 | 101 | core::dbus::Bus::Ptr bus; | 103 | core::dbus::Bus::Ptr bus; |
PASSED: Continuous integration, rev:148 jenkins. qa.ubuntu. com/job/ location- service- ci/329/ jenkins. qa.ubuntu. com/job/ location- service- vivid-amd64- ci/1 jenkins. qa.ubuntu. com/job/ location- service- vivid-armhf- ci/1 jenkins. qa.ubuntu. com/job/ location- service- vivid-armhf- ci/1/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ location- service- vivid-i386- ci/1
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/location- service- ci/329/ rebuild
http://