Merge lp:~thomas-voss/location-service/fix-match-rule-leak into lp:location-service/15.04

Proposed by Thomas Voß on 2015-11-10
Status: Merged
Approved by: Alberto Mardegan on 2015-11-23
Approved revision: 204
Merged at revision: 207
Proposed branch: lp:~thomas-voss/location-service/fix-match-rule-leak
Merge into: lp:location-service/15.04
Diff against target: 117 lines (+35/-11)
4 files modified
src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.cpp (+12/-6)
src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.h (+12/-0)
src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp (+1/-1)
src/location_service/com/ubuntu/location/service/provider_daemon.cpp (+10/-4)
To merge this branch: bzr merge lp:~thomas-voss/location-service/fix-match-rule-leak
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) 2015-11-10 Approve on 2015-11-23
Review via email: mp+277179@code.launchpad.net

Commit message

Ensure that event connections are cleaned up on destruction.

Description of the change

Ensure that event connections are cleaned up on destruction.

To post a comment you must log in.
199. By Thomas Voß on 2015-11-12

Instrument on_access_point_{added, removed} with log messages.

200. By Thomas Voß on 2015-11-12

One connection is more than enough.

201. By Thomas Voß on 2015-11-12

Don't add, just use an existing object.

202. By Thomas Voß on 2015-11-12

Log destruction of CachedWirelessNetwork instances.

203. By Thomas Voß on 2015-11-17

Remove temporary log output.

204. By Thomas Voß on 2015-11-17

Revert accidental change to debian/source/format.

Alberto Mardegan (mardy) wrote :

The last chunk of changes looks unrelated and it's not obvious whether what it's solving. But if that's not an accidental change, it all looks good to me.

review: Approve
205. By Thomas Voß on 2015-12-02

[ Alberto Mardegan ]
* Send last known position on session start
[ CI Train Bot ]
* New rebuild forced.
[ Thomas Voß ]
* Factor out service::Runtime from daemon.cpp into its own .h/.cpp
  pair of files. Add test cases around correct operation of
  service::Runtime. added:
  src/location_service/com/ubuntu/location/service/runtime.cpp
  src/location_service/com/ubuntu/location/service/runtime.h
  tests/runtime_test.cpp
[ thomas-voss ]
* Factor out service::Runtime from daemon.cpp into its own .h/.cpp
  pair of files. Add test cases around correct operation of
  service::Runtime. added:
  src/location_service/com/ubuntu/location/service/runtime.cpp
  src/location_service/com/ubuntu/location/service/runtime.h
  tests/runtime_test.cpp
* Adjust default timeout for downloading GPS XTRA data.
[ Alberto Mardegan ]
* Make sure that injected time is given in milliseconds
[ Thomas Voß ]
* Cherry-pick rev. 196 and 199 from lp:location-service. The changes
  got accidentally removed by merging the outstanding documentation
  branch.
* Handle responses of clients to updates asynchronously. Rely on
  dummy::ConnectivityManager as harvesting is disabled anyway. (LP:
  #1462664, #1387643)

206. By Thomas Voß on 2015-12-02

Revert accidental changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.cpp'
2--- src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.cpp 2015-05-28 10:57:24 +0000
3+++ src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.cpp 2015-12-02 12:11:29 +0000
4@@ -123,7 +123,14 @@
5 const org::freedesktop::NetworkManager::Device& device,
6 const org::freedesktop::NetworkManager::AccessPoint& ap)
7 : device_(device),
8- access_point_(ap)
9+ access_point_(ap),
10+ connections
11+ {
12+ access_point_.properties_changed->connect([this](const std::map<std::string, core::dbus::types::Variant>& dict)
13+ {
14+ on_access_point_properties_changed(dict);
15+ })
16+ }
17 {
18 last_seen_ = translate_time_stamp(access_point_.last_seen->get());
19
20@@ -138,12 +145,11 @@
21 {
22 static_cast<int>(access_point_.strength->get())
23 };
24+}
25
26- // Wire up all the connections
27- access_point_.properties_changed->connect([this](const std::map<std::string, core::dbus::types::Variant>& dict)
28- {
29- on_access_point_properties_changed(dict);
30- });
31+detail::CachedWirelessNetwork::~CachedWirelessNetwork()
32+{
33+ access_point_.properties_changed->disconnect(connections.ap_properties_changed);
34 }
35
36 void detail::CachedWirelessNetwork::on_access_point_properties_changed(const std::map<std::string, core::dbus::types::Variant>& dict)
37
38=== modified file 'src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.h'
39--- src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.h 2014-08-14 20:25:22 +0000
40+++ src/location_service/com/ubuntu/location/connectivity/cached_wireless_network.h 2015-12-02 12:11:29 +0000
41@@ -41,6 +41,8 @@
42 const org::freedesktop::NetworkManager::Device& device,
43 const org::freedesktop::NetworkManager::AccessPoint& ap);
44
45+ ~CachedWirelessNetwork();
46+
47 // Timestamp when the network became visible.
48 const core::Property<std::chrono::system_clock::time_point>& last_seen() const override;
49
50@@ -67,6 +69,16 @@
51 // The actual access point stub.
52 org::freedesktop::NetworkManager::AccessPoint access_point_;
53
54+ // Encapsulates all event connections that have to be cut on destruction.
55+ struct
56+ {
57+ core::dbus::Signal
58+ <
59+ org::freedesktop::NetworkManager::AccessPoint::PropertiesChanged,
60+ org::freedesktop::NetworkManager::AccessPoint::PropertiesChanged::ArgumentType
61+ >::SubscriptionToken ap_properties_changed;
62+ } connections;
63+
64 core::Property<std::chrono::system_clock::time_point> last_seen_;
65 core::Property<std::string> bssid_;
66 core::Property<std::string> ssid_;
67
68=== modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
69--- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2015-05-27 18:40:37 +0000
70+++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2015-12-02 12:11:29 +0000
71@@ -595,7 +595,7 @@
72
73 xdg::NetworkManager::AccessPoint ap
74 {
75- network_manager->service->add_object_for_path(ap_path)
76+ network_manager->service->object_for_path(ap_path)
77 };
78
79 auto wifi = std::make_shared<detail::CachedWirelessNetwork>(itd->second, ap);
80
81=== modified file 'src/location_service/com/ubuntu/location/service/provider_daemon.cpp'
82--- src/location_service/com/ubuntu/location/service/provider_daemon.cpp 2015-11-25 09:20:12 +0000
83+++ src/location_service/com/ubuntu/location/service/provider_daemon.cpp 2015-12-02 12:11:29 +0000
84@@ -108,6 +108,14 @@
85 return result;
86 }
87
88+namespace
89+{
90+std::shared_ptr<location::service::Runtime> runtime()
91+{
92+ static const auto inst = location::service::Runtime::create(1);
93+ return inst;
94+}
95+}
96 int location::service::ProviderDaemon::main(const location::service::ProviderDaemon::Configuration& config)
97 {
98 auto trap = core::posix::trap_signals_for_all_subsequent_threads(
99@@ -121,9 +129,7 @@
100 trap->stop();
101 });
102
103- auto runtime = location::service::Runtime::create(1);
104-
105- config.connection->install_executor(core::dbus::asio::make_executor(config.connection, runtime->service()));
106+ config.connection->install_executor(core::dbus::asio::make_executor(config.connection, runtime()->service()));
107
108 auto skeleton = location::providers::remote::skeleton::create_with_configuration(location::providers::remote::skeleton::Configuration
109 {
110@@ -132,7 +138,7 @@
111 config.provider
112 });
113
114- runtime->start();
115+ runtime()->start();
116
117 trap->run();
118

Subscribers

People subscribed via source and target branches