diff --git a/tests/test_contact.py b/tests/test_contact.py index 52fe010..de84848 100644 --- a/tests/test_contact.py +++ b/tests/test_contact.py @@ -169,9 +169,8 @@ class Contact(Base): with mock.patch.object(tm, "post", side_effect=remove_meanwhile): fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True}) self.assertNotIn("me@example.com", tm.SENT.read_text()) - fc._removed.clear() - fr.send({**REPORT, "contact": "other@example.com", "contactFollowup": True}) - self.assertIn("other@example.com", tm.SENT.read_text()) # other reports are logged as sent + fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True}) + self.assertIn("me@example.com", tm.SENT.read_text()) # sent again after removal: logged as sent def test_saving_during_a_slow_send_returns_at_once(self): busy = fc._send_lock @@ -185,6 +184,28 @@ class Contact(Base): self.assertTrue(fc._send_pending()) self.assertEqual(len(self.got), 1) + def test_a_change_saved_as_a_send_finishes_is_not_left_behind(self): + real = fc._send_lock + + class Lock: # a Save lands after the sender found nothing waiting, before it lets go + saved = False + + def acquire(self, blocking=True): + return real.acquire(blocking) + + def release(self): + if not Lock.saved: + Lock.saved = True + s = threading.Thread(target=fc.save, args=({"email": "me@example.com", "updates": True},)) + s.start() + s.join(5) + real.release() + + with mock.patch.object(fc, "_send_lock", Lock()): + self.assertTrue(fc._send_pending()) + self.assertEqual([e["properties"]["email"] for e in self.events()], ["me@example.com"]) + self.assertFalse(fc.state()["waiting"]) + # ---- the one-time prompt def test_the_prompt_waits_for_a_working_setup_then_stays_dismissed(self): diff --git a/ui/frame_contact.py b/ui/frame_contact.py index 3586337..c79e2c1 100644 --- a/ui/frame_contact.py +++ b/ui/frame_contact.py @@ -18,7 +18,6 @@ A change that can't be sent (offline) waits in the state file and is retried in background, so a withdrawal is never lost. The page's one-time prompt is remembered here too: once it has been shown or dismissed it never comes back. """ -import calendar import json import os import re @@ -100,7 +99,7 @@ def _send_pending(block=True): with _lock: event = load()['pending'] if event is None: - return True + break try: frame_telemetry.post([event], timeout=30) except frame_telemetry.SendError: @@ -108,6 +107,10 @@ def _send_pending(block=True): _sent(event) finally: _send_lock.release() + # A change saved just as this finished found the lock still held and left it to us. + with _lock: + left = load()['pending'] is not None + return _send_pending(block=False) if left else True def _sent(event): @@ -139,18 +142,14 @@ def _forget_locally(email): frame_telemetry._write_lines(frame_telemetry.SENT, rows) -def redact_removed(event): - """Before logging a report sent while its address was being removed: take the address out. - Call with frame_telemetry._lock held, so a removal can't slip between this and the log.""" +def redact_removed(event, started): + """Before logging a report (started at time.time() `started`) whose address was removed + while it was being sent: take the address out. Call with frame_telemetry._lock held, so a + removal can't slip between this and the log.""" p = event.get('properties') or {} removed_at = _removed.get(str(p.get('contact') or '').strip().lower()) - if removed_at is not None: - try: - started = calendar.timegm(time.strptime(event['timestamp'], '%Y-%m-%dT%H:%M:%SZ')) - except (KeyError, ValueError): - started = 0 - if started <= removed_at: - p['contact'] = '' + if removed_at is not None and started <= removed_at: + p['contact'] = '' def save(body): diff --git a/ui/frame_report.py b/ui/frame_report.py index 1c3f8f2..25b887c 100644 --- a/ui/frame_report.py +++ b/ui/frame_report.py @@ -123,13 +123,14 @@ def send(body): # Its own random id: a report can carry contact details, so it isn't linked to this copy's analytics. event = {'event': 'problem_report', 'distinct_id': str(uuid.uuid4()), 'uuid': str(uuid.uuid4()), 'timestamp': time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime()), 'properties': props} + started = time.time() try: frame_telemetry.post([event], timeout=30) except frame_telemetry.SendError as e: raise ReportError(str(e)) try: with frame_telemetry._lock: # the lock a removal holds while wiping its address - frame_contact.redact_removed(event) + frame_contact.redact_removed(event, started) frame_telemetry.record_sent([event]) except OSError: pass # it was sent; failing to log it here mustn't make the person send it again