Merge lp:~facundo/ubuntuone-client/fix-conflict-case into lp:ubuntuone-client

Proposed by Facundo Batista
Status: Merged
Approved by: Facundo Batista
Approved revision: 905
Merged at revision: 907
Proposed branch: lp:~facundo/ubuntuone-client/fix-conflict-case
Merge into: lp:ubuntuone-client
Diff against target: 369 lines (+137/-27)
2 files modified
tests/syncdaemon/test_action_queue.py (+108/-11)
ubuntuone/syncdaemon/action_queue.py (+29/-16)
To merge this branch: bzr merge lp:~facundo/ubuntuone-client/fix-conflict-case
Reviewer Review Type Date Requested Status
Facundo Batista (community) Approve
John O'Brien (community) Approve
Review via email: mp+52255@code.launchpad.net

Commit message

Avoid autoconflict when saving several times the same file (LP: #718924)

Description of the change

Avoid autoconflict when saving several times the same file

This happened because when an Upload was cancelled, it was cancelled and removed from the queue no matter what. However, if the EOF was already sent to the server (the producer already finished), the Upload should not be cancelled, and let it finish.

Also in this branch I normalized the Upload/Download treatment of other similar commands: they do not try to cancel stuff at __init__ anymore, they just cancel other commands they find when being asked if they can be queued.

Finally, I added a log line when a command is cancelled, and enhanced the one in the external cancel.

Tests included for everything, but if you want to do an extensive test, you can check a script I added in the bug related to this branch. That script just saves a lot of times a file, changing the period between saves, which generated the autoconflicts previously to this branch.

To post a comment you must log in.
Revision history for this message
John O'Brien (jdobrien) wrote :

Nice fix.

review: Approve
Revision history for this message
Facundo Batista (facundo) wrote :

Approving with one review.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/syncdaemon/test_action_queue.py'
2--- tests/syncdaemon/test_action_queue.py 2011-03-02 19:21:34 +0000
3+++ tests/syncdaemon/test_action_queue.py 2011-03-04 21:07:21 +0000
4@@ -181,6 +181,11 @@
5 """Fake Request."""
6 def __init__(self):
7 self.deferred = defer.succeed(True)
8+ self.cancelled = False
9+
10+ def cancel(self):
11+ """Mark cancelled."""
12+ self.cancelled = True
13
14
15 class FakeClient(object):
16@@ -343,7 +348,7 @@
17 """It does not cancel anything, because all empty."""
18 assert not self.action_queue.queue.waiting
19 self.action_queue._cancel_op('shr', 'node', Upload)
20- self.assertFalse(self.handler.check_debug('cancelled', 'shr', 'node'))
21+ self.assertFalse(self.handler.check_debug('cancel', 'shr', 'node'))
22
23 def _set_queue(self, *waiting):
24 """Set the content queue content."""
25@@ -358,7 +363,7 @@
26 cmd2 = FakeCommand('sh', 'nd2')
27 self._set_queue(cmd1, cmd2)
28 self.action_queue._cancel_op('sh', 'nd3', FakeCommand)
29- self.assertFalse(self.handler.check_debug('cancelled', 'sh', 'nd3'))
30+ self.assertFalse(self.handler.check_debug('external cancel attempt'))
31 self.assertFalse(cmd1.cancelled)
32 self.assertFalse(cmd2.cancelled)
33
34@@ -368,7 +373,7 @@
35 cmd2 = FakeCommand('sh', 'nd')
36 self._set_queue(cmd1, cmd2)
37 self.action_queue._cancel_op('sh', 'nd', Upload)
38- self.assertFalse(self.handler.check_debug('cancelled', 'sh', 'nd'))
39+ self.assertFalse(self.handler.check_debug('external cancel attempt'))
40 self.assertFalse(cmd1.cancelled)
41 self.assertFalse(cmd2.cancelled)
42
43@@ -377,7 +382,8 @@
44 cmd = FakeCommand('sh', 'nd')
45 self._set_queue(cmd)
46 self.action_queue._cancel_op('sh', 'nd', FakeCommand)
47- self.assertTrue(self.handler.check_debug('cancelled', 'sh', 'nd'))
48+ self.assertTrue(self.handler.check_debug('external cancel attempt',
49+ 'sh', 'nd'))
50 self.assertTrue(cmd.cancelled)
51
52 def test_node_is_queued_move_api(self):
53@@ -2080,9 +2086,11 @@
54 self.cmd.cleanup = lambda: called.append(1)
55 self.cmd.finish = lambda: called.append(2)
56 assert not self.cmd.cancelled
57- self.cmd.cancel()
58+ did_cancel = self.cmd.cancel()
59+ self.assertTrue(did_cancel)
60 self.assertEqual(called, [1, 2])
61 self.assertTrue(self.cmd.cancelled)
62+ self.assertTrue(self.handler.check_debug('cancelled'))
63
64 def test_cancel_releases_conditions(self):
65 """Cancel unlocks the conditions deferred."""
66@@ -2098,7 +2106,8 @@
67 self.cmd.cleanup = lambda: called.append(True)
68 self.cmd.finish = lambda: called.append(True)
69 self.cmd.cancelled = True
70- self.cmd.cancel()
71+ did_cancel = self.cmd.cancel()
72+ self.assertFalse(did_cancel)
73 self.assertFalse(called)
74 self.assertTrue(self.cmd.cancelled)
75
76@@ -2743,13 +2752,15 @@
77 def test_cancel_set_cancelled(self):
78 """Set the command to cancelled."""
79 assert not self.command.cancelled, "test badly set up"
80- self.command.cancel()
81+ did_cancel = self.command.cancel()
82+ self.assertTrue(did_cancel)
83 self.assertTrue(self.command.cancelled)
84
85 def test_cancel_download_req_is_none(self):
86 """It's ok to have download_req in None when cancelling."""
87 assert self.command.download_req is None, "test badly set up"
88- self.command.cancel()
89+ did_cancel = self.command.cancel()
90+ self.assertTrue(did_cancel)
91
92 def test_cancel_download_req_is_something(self):
93 """download_req is also cancelled."""
94@@ -2799,30 +2810,55 @@
95 """There should be only one upload/download for a specific node."""
96 # totally fake, we don't care: the messages are only validated on run
97 self.action_queue.download('foo', 'bar', 0, 'path', 0)
98- self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path', 0)
99+ first_cmd = self.action_queue.queue.waiting[0]
100+ self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path', StringIO)
101 self.assertEqual(len(self.action_queue.queue.waiting), 1)
102+ self.assertTrue(first_cmd.cancelled)
103
104 def test_uniqueness_upload(self):
105 """There should be only one upload/download for a specific node."""
106 # totally fake, we don't care: the messages are only validated on run
107- self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path', 0)
108+ self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path', StringIO)
109+ first_cmd = self.action_queue.queue.waiting[0]
110 self.action_queue.download('foo', 'bar', 0, 'path', 0)
111 self.assertEqual(len(self.action_queue.queue.waiting), 1)
112+ self.assertTrue(first_cmd.cancelled)
113+ self.assertTrue(self.handler.check_debug("Previous command cancelled",
114+ "Upload", "foo", "bar"))
115
116 def test_uniqueness_download(self):
117 """There should be only one upload/download for a specific node."""
118 # totally fake, we don't care: the messages are only validated on run
119 self.action_queue.download('foo', 'bar', 0, 'path0', 0)
120+ first_cmd = self.action_queue.queue.waiting[0]
121 self.action_queue.download('foo', 'bar', 1, 'path1', 1)
122 self.assertEqual(len(self.action_queue.queue.waiting), 1)
123+ self.assertTrue(first_cmd.cancelled)
124+ self.assertTrue(self.handler.check_debug("Previous command cancelled",
125+ "Download", "foo", "bar"))
126
127 def test_uniqueness_even_with_markers(self):
128 """Only one upload/download per node, even using markers."""
129 m = MDMarker('bar')
130 self.action_queue.download('share', m, 0, 'path', 0)
131+ first_cmd = self.action_queue.queue.waiting[0]
132 self.action_queue.uuid_map.set('bar', 'bah')
133 self.action_queue.download('share', 'bah', 0, 'path', 0)
134 self.assertEqual(len(self.action_queue.queue.waiting), 1)
135+ self.assertTrue(first_cmd.cancelled)
136+
137+ def test_uniqueness_tried_to_cancel_but_no(self):
138+ """Previous command didn't cancel even if we tried it."""
139+ # the first command will refuse to cancel (patch the class because
140+ # the instance is not patchable)
141+ self.action_queue.download('foo', 'bar', 0, 'path0', 0)
142+ self.action_queue.queue.waiting[0]
143+ self.patch(Download, 'cancel', lambda instance: False)
144+
145+ self.action_queue.download('foo', 'bar', 1, 'path1', 1)
146+ self.assertEqual(len(self.action_queue.queue.waiting), 2)
147+ self.assertTrue(self.handler.check_debug("Tried to cancel", "couldn't",
148+ "Download", "foo", "bar"))
149
150 def test_start_locks_on_semaphore(self):
151 """_start acquire the semaphore and locks."""
152@@ -3226,7 +3262,45 @@
153 def test_cancel_upload_req_is_none(self):
154 """It's ok to have upload_req in None when cancelling."""
155 assert self.command.upload_req is None, "test badly set up"
156- self.command.cancel()
157+ did_cancel = self.command.cancel()
158+ self.assertTrue(did_cancel)
159+
160+ def test_cancel_abort_when_producer_finished(self):
161+ """If the producer already finished, don't really cancel."""
162+ called = []
163+ self.patch(ActionQueueCommand, 'cancel', lambda s: called.append(1))
164+
165+ class FakeProducer(object):
166+ """Fake producer."""
167+ finished = True
168+
169+ fake_request = FakeRequest()
170+ fake_request.producer = FakeProducer()
171+ self.command.upload_req = fake_request
172+
173+ did_cancel = self.command.cancel()
174+ self.assertFalse(did_cancel)
175+ self.assertFalse(called)
176+ self.assertFalse(fake_request.cancelled)
177+
178+ def test_cancel_cancels_when_producer_not_finished(self):
179+ """If the producer didn't finished, really cancel."""
180+ called = []
181+ self.patch(ActionQueueCommand, 'cancel',
182+ lambda s: called.append(True) or True)
183+
184+ class FakeProducer(object):
185+ """Fake producer."""
186+ finished = False
187+
188+ fake_request = FakeRequest()
189+ fake_request.producer = FakeProducer()
190+ self.command.upload_req = fake_request
191+
192+ did_cancel = self.command.cancel()
193+ self.assertTrue(did_cancel)
194+ self.assertTrue(called)
195+ self.assertTrue(fake_request.cancelled)
196
197 def test_cancel_upload_req_is_something(self):
198 """upload_req is also cancelled."""
199@@ -3235,6 +3309,7 @@
200 obj = mocker.mock()
201 obj.cancel()
202 obj.producer
203+ obj.producer
204
205 # test
206 with mocker:
207@@ -3277,23 +3352,45 @@
208 """There should be only one upload/download for a specific node."""
209 # totally fake, we don't care: the messages are only validated on run
210 self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path0', StringIO)
211+ first_cmd = self.action_queue.queue.waiting[0]
212 self.action_queue.upload('foo', 'bar', 1, 1, 1, 1, 'path1', StringIO)
213 self.assertEqual(len(self.action_queue.queue.waiting), 1)
214+ self.assertTrue(first_cmd.cancelled)
215+ self.assertTrue(self.handler.check_debug("Previous command cancelled",
216+ "Upload", "foo", "bar"))
217
218 def test_uniqueness_download(self):
219 """There should be only one upload/download for a specific node."""
220 # totally fake, we don't care: the messages are only validated on run
221 self.action_queue.download('foo', 'bar', 0, 'path', StringIO)
222+ first_cmd = self.action_queue.queue.waiting[0]
223 self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path', StringIO)
224 self.assertEqual(len(self.action_queue.queue.waiting), 1)
225+ self.assertTrue(first_cmd.cancelled)
226+ self.assertTrue(self.handler.check_debug("Previous command cancelled",
227+ "Download", "foo", "bar"))
228
229 def test_uniqueness_even_with_markers(self):
230 """Only one upload/download per node, even using markers."""
231 m = MDMarker('bar')
232 self.action_queue.download('share', m, 0, 'path', StringIO)
233+ first_cmd = self.action_queue.queue.waiting[0]
234 self.action_queue.uuid_map.set('bar', 'bah')
235 self.action_queue.upload('share', 'bah', 0, 0, 0, 0, 'path', StringIO)
236 self.assertEqual(len(self.action_queue.queue.waiting), 1)
237+ self.assertTrue(first_cmd.cancelled)
238+
239+ def test_uniqueness_tried_to_cancel_but_no(self):
240+ """Previous command didn't cancel even if we tried it."""
241+ # the first command will refuse to cancel
242+ self.action_queue.upload('foo', 'bar', 0, 0, 0, 0, 'path0', StringIO)
243+ self.action_queue.queue.waiting[0]
244+ self.patch(Upload, 'cancel', lambda instance: False)
245+
246+ self.action_queue.upload('foo', 'bar', 1, 1, 1, 1, 'path1', StringIO)
247+ self.assertEqual(len(self.action_queue.queue.waiting), 2)
248+ self.assertTrue(self.handler.check_debug("Tried to cancel", "couldn't",
249+ "Upload", "foo", "bar"))
250
251 def test_start_locks_on_semaphore(self):
252 """_start acquire the semaphore and locks."""
253
254=== modified file 'ubuntuone/syncdaemon/action_queue.py'
255--- ubuntuone/syncdaemon/action_queue.py 2011-03-02 19:21:34 +0000
256+++ ubuntuone/syncdaemon/action_queue.py 2011-03-04 21:07:21 +0000
257@@ -1043,11 +1043,9 @@
258 logstr = "cancel_" + cmdclass.__name__.lower()
259 log = mklog(logger, logstr, share_id, node_id)
260 uniqueness = (cmdclass.__name__, share_id, node_id)
261-
262- # check waiting
263 if uniqueness in self.queue.hashed_waiting:
264 queued_command = self.queue.hashed_waiting[uniqueness]
265- log.debug('cancelled')
266+ log.debug('external cancel attempt')
267 queued_command.cancel()
268
269 def cancel_upload(self, share_id, node_id):
270@@ -1322,16 +1320,20 @@
271 Do nothing if already cancelled (as cancellation can come from other
272 thread, it can come at any time, so we need to support double
273 cancellation safely).
274+
275+ Return True if the command was really cancelled.
276 """
277 if self.cancelled:
278- return
279+ return False
280
281 self.cancelled = True
282+ self.log.debug('cancelled')
283 if self.wait_for_conditions is not None:
284 self.wait_for_conditions.callback(True)
285 self.wait_for_conditions = None
286 self.cleanup()
287 self.finish()
288+ return True
289
290 def _acquire_pathlock(self):
291 """Acquire pathlock; overwrite if needed."""
292@@ -2122,7 +2124,6 @@
293 self.fileobj_factory = fileobj_factory
294 self.fileobj = None
295 self.gunzip = None
296- self.action_queue.cancel_download(self.share_id, self.node_id)
297 self.path = path
298 self.download_req = None
299 self.n_bytes_read = 0
300@@ -2140,10 +2141,14 @@
301 for uniq in [(Upload.__name__, self.share_id, self.node_id),
302 (Download.__name__, self.share_id, self.node_id)]:
303 if uniq in self._queue.hashed_waiting:
304- previous_command = self._queue.hashed_waiting.pop(uniq)
305- self._queue.waiting.remove(previous_command)
306- m = "removing previous command because uniqueness: %s"
307- logger.debug(m, previous_command)
308+ previous_command = self._queue.hashed_waiting[uniq]
309+ did_cancel = previous_command.cancel()
310+ if did_cancel:
311+ m = "Previous command cancelled because uniqueness: %s"
312+ else:
313+ m = ("Tried to cancel other command because uniqueness, "
314+ "but couldn't: %s")
315+ self.log.debug(m, previous_command)
316 return True
317
318 def _acquire_pathlock(self):
319@@ -2155,7 +2160,7 @@
320 """Cancel the download."""
321 if self.download_req is not None:
322 self.download_req.cancel()
323- super(Download, self).cancel()
324+ return super(Download, self).cancel()
325
326 @defer.inlineCallbacks
327 def _start(self):
328@@ -2299,7 +2304,6 @@
329 self.tempfile_factory = tempfile_factory
330 self.upload_id = upload_id
331 self.tempfile = None
332- self.action_queue.cancel_upload(self.share_id, self.node_id)
333 self.path = path
334 self.upload_req = None
335 self.n_bytes_written_last = 0
336@@ -2327,10 +2331,14 @@
337 for uniq in [(Upload.__name__, self.share_id, self.node_id),
338 (Download.__name__, self.share_id, self.node_id)]:
339 if uniq in self._queue.hashed_waiting:
340- previous_command = self._queue.hashed_waiting.pop(uniq)
341- self._queue.waiting.remove(previous_command)
342- m = "removing previous command because uniqueness: %s"
343- logger.debug(m, previous_command)
344+ previous_command = self._queue.hashed_waiting[uniq]
345+ did_cancel = previous_command.cancel()
346+ if did_cancel:
347+ m = "Previous command cancelled because uniqueness: %s"
348+ else:
349+ m = ("Tried to cancel other command because uniqueness, "
350+ "but couldn't: %s")
351+ self.log.debug(m, previous_command)
352 return True
353
354 @property
355@@ -2346,8 +2354,13 @@
356 def cancel(self):
357 """Cancel the upload."""
358 if self.upload_req is not None:
359+ producer = self.upload_req.producer
360+ if producer is not None and producer.finished:
361+ # can not cancel if already sent the EOF
362+ return False
363+
364 self.upload_req.cancel()
365- super(Upload, self).cancel()
366+ return super(Upload, self).cancel()
367
368 def cleanup(self):
369 """Cleanup: stop the producer."""

Subscribers

People subscribed via source and target branches