Merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-publishapi into lp:ubuntuone-client

Proposed by Diego Sarmentero on 2012-10-05
Status: Merged
Approved by: Diego Sarmentero on 2012-10-09
Approved revision: 1340
Merged at revision: 1336
Proposed branch: lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-publishapi
Merge into: lp:ubuntuone-client
Diff against target: 180 lines (+113/-11)
2 files modified
tests/platform/tools/test_tools.py (+98/-6)
ubuntuone/platform/tools/perspective_broker.py (+15/-5)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-publishapi
Reviewer Review Type Date Requested Status
Manuel de la Peña (community) Approve on 2012-10-09
Mike McCracken (community) 2012-10-05 Approve on 2012-10-09
Review via email: mp+128312@code.launchpad.net

Commit message

- Making the handler connection to be a list of handlers (LP: #1061880).

To post a comment you must log in.
Mike McCracken (mikemc) wrote :

This is related to this control-panel branch:
https://code.launchpad.net/~diegosarmentero/ubuntuone-control-panel/u1-cp-publishapi/+merge/128316

but in light of that branch, it's not totally clear why we need to change this here.

share_links is the only part of control panel that ever sets signal handlers for the public_files handler and the public_access_changed handler. Is it possible to have multiple invocations of this remote call from control panel? ie, does this happen if we click fast to share two files in the table or something? or is that impossible?

Also, just a note that on IRC I requested a test that shows the problem and breaks with the old code… Diego was working on that at EOD friday…

review: Needs Information
Diego Sarmentero (diegosarmentero) wrote :

We needed to store a list of handlers to be called, because the setattr is overriding the behaviour of that part of the object every time a connect_signal is requested, and it was happening that several functions were being connected to the same signals, the ones with the handler that we actually want, and some other for clean up at syncdaemon core, and only the last handler connected was the one that was being called.

1335. By Diego Sarmentero on 2012-10-08

merge

1336. By Diego Sarmentero on 2012-10-09

improving handler calls

Manuel de la Peña (mandel) wrote :

73 + self.addCleanup(self.sdtool.shutdown)

Why not adding it after the creation of the sdtool?

review: Needs Fixing
1337. By Diego Sarmentero on 2012-10-09

improve test

1338. By Diego Sarmentero on 2012-10-09

fix test

1339. By Diego Sarmentero on 2012-10-09

adding new test

Mike McCracken (mikemc) wrote :

minor - test_connect_avoid_repeated_connection has the same docstring as test_connect_several_handlers_to_same_signal but they test different things

review: Needs Fixing
1340. By Diego Sarmentero on 2012-10-09

fixed docstring

Mike McCracken (mikemc) wrote :

Approve. These new tests pass on OSX*, and testing the share links panel shows no further hangs.

* the whole suite still has a ton of dirty reactor errors though

review: Approve
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/platform/tools/test_tools.py'
2--- tests/platform/tools/test_tools.py 2012-08-27 15:22:13 +0000
3+++ tests/platform/tools/test_tools.py 2012-10-09 15:37:25 +0000
4@@ -139,11 +139,11 @@
5 def method_call(self, *args, **kwargs):
6 """Fake a remote method call."""
7 if self.number_calls == 0:
8- self.number_calls += 1
9- raise pb.DeadReferenceError()
10+ self.number_calls += 1
11+ raise pb.DeadReferenceError()
12 else:
13- self.called.append((args, kwargs))
14- return defer.succeed(self.number_calls)
15+ self.called.append((args, kwargs))
16+ return defer.succeed(self.number_calls)
17
18
19 class PerspectiveBrokerReconnect(TestCase):
20@@ -184,8 +184,100 @@
21 @defer.inlineCallbacks
22 def test_reconnect_signals(self):
23 """Test reconnection with signals."""
24- self.sdtool.connected_signals = dict(create_signal=lambda:None,
25- delete_signal=lambda:None)
26+ self.sdtool.connected_signals = dict(create_signal=[lambda: None],
27+ delete_signal=[lambda: None])
28 yield self.sdtool.call_method('fake_remote', 'method_call')
29 self.assertTrue(self.reconnected)
30 self.assertEqual(2, len(self.connected_signals))
31+
32+
33+class FakeU1Objects(object):
34+ """Fake PublicFiles for UbuntuOneClient."""
35+
36+ def on_public_files_list_cb(self):
37+ """Do nothing."""
38+
39+ def on_share_changed_cb(self):
40+ """Do nothing."""
41+
42+
43+class FakeUbuntuOneClient(object):
44+ """Fake UbuntuOneClient."""
45+
46+ def __init__(self):
47+ self.public_files = FakeU1Objects()
48+ self.shares = FakeU1Objects()
49+ self.connected = True
50+
51+ def is_connected(self):
52+ """Fake is_connected."""
53+ return self.connected
54+
55+ def connect(self):
56+ """Fake connect."""
57+ self.connected = True
58+ yield self.connected
59+
60+ def disconnect(self):
61+ """Fake disconnect."""
62+ self.connected = False
63+
64+
65+class PerspectiveBrokerConnectSignal(TestCase):
66+ """Test when the ipc is reconnected."""
67+
68+ @defer.inlineCallbacks
69+ def setUp(self):
70+ """Set the tests."""
71+ yield super(PerspectiveBrokerConnectSignal, self).setUp()
72+ self.patch(perspective_broker, 'UbuntuOneClient', FakeUbuntuOneClient)
73+ self.sdtool = perspective_broker.SyncDaemonToolProxy()
74+ self.addCleanup(self.sdtool.shutdown)
75+
76+ def test_connect_several_handlers_to_same_signal(self):
77+ """Connect more than one handler to the same signal name."""
78+ data = []
79+
80+ signal_name = "PublicFilesList"
81+ self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
82+ self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
83+
84+ self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 2)
85+ self.sdtool.client.public_files.on_public_files_list_cb()
86+ expected = [(), ()]
87+ self.assertEqual(data, expected)
88+
89+ def test_connect_avoid_repeated_connection(self):
90+ """Ensure that we don't have the same handler called twice."""
91+ data = []
92+
93+ signal_name = "PublicFilesList"
94+ func = lambda *a: data.append(a)
95+ self.sdtool.connect_signal(signal_name, func)
96+ self.sdtool.connect_signal(signal_name, func)
97+
98+ self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 1)
99+ self.sdtool.client.public_files.on_public_files_list_cb()
100+ expected = [()]
101+ self.assertEqual(data, expected)
102+
103+ def test_proper_connections(self):
104+ """Check that the proper handlers are called."""
105+ data = []
106+ data2 = []
107+
108+ signal_name = "PublicFilesList"
109+ signal_name2 = "ShareChanges"
110+ self.sdtool.connect_signal(signal_name, lambda *a: data.append(a))
111+ self.sdtool.connect_signal(signal_name2, lambda *a: data2.append(a))
112+
113+ self.assertEqual(len(self.sdtool.connected_signals[signal_name]), 1)
114+ self.assertEqual(len(self.sdtool.connected_signals[signal_name2]), 1)
115+ self.sdtool.client.public_files.on_public_files_list_cb()
116+ expected = [()]
117+ self.assertEqual(data, expected)
118+ self.assertEqual(data2, [])
119+ data = []
120+ self.sdtool.client.shares.on_share_changed_cb()
121+ self.assertEqual(data, [])
122+ self.assertEqual(data2, expected)
123\ No newline at end of file
124
125=== modified file 'ubuntuone/platform/tools/perspective_broker.py'
126--- ubuntuone/platform/tools/perspective_broker.py 2012-08-27 15:49:13 +0000
127+++ ubuntuone/platform/tools/perspective_broker.py 2012-10-09 15:37:25 +0000
128@@ -30,6 +30,7 @@
129
130 import logging
131 import subprocess
132+from collections import defaultdict
133
134 from twisted.internet import defer
135 from twisted.spread.pb import DeadReferenceError
136@@ -122,7 +123,7 @@
137 'perspective_broker')
138 self.client = UbuntuOneClient()
139 self.connected = None
140- self.connected_signals = {}
141+ self.connected_signals = defaultdict(set)
142
143 def _call_after_connection(self, method):
144 """Make sure Perspective Broker is connected before calling."""
145@@ -150,8 +151,9 @@
146 self.connected = False
147 yield self.client.reconnect()
148 # do connect all the signals again
149- for signal_name, handler in self.connected_signals.items():
150- self.connect_signal(signal_name, handler)
151+ for signal_name, handlers in self.connected_signals.items():
152+ for handler in handlers:
153+ self.connect_signal(signal_name, handler)
154
155 @defer.inlineCallbacks
156 def call_method(self, client_kind, method_name, *args, **kwargs):
157@@ -173,13 +175,21 @@
158 """Close connections."""
159 return self.client.disconnect()
160
161+ def _handler(self, signal_name, *args, **kwargs):
162+ """Call all the handlers connected to signal_name."""
163+ for cb_handler in self.connected_signals[signal_name]:
164+ cb_handler(*args, **kwargs)
165+
166 def connect_signal(self, signal_name, handler):
167 """Connect 'handler' with 'signal_name'."""
168 client_kind, callback = self._SIGNAL_MAPPING[signal_name]
169 client = getattr(self.client, client_kind)
170- setattr(client, callback, handler)
171+ if len(self.connected_signals[signal_name]) == 0:
172+ setattr(client, callback,
173+ lambda *args, **kwargs:
174+ self._handler(signal_name, *args, **kwargs))
175 # do remember the connected signal in case we need to reconnect
176- self.connected_signals[signal_name] = handler
177+ self.connected_signals[signal_name].add(handler)
178 return handler
179
180 def disconnect_signal(self, signal_name, handler_or_match):

Subscribers

People subscribed via source and target branches