From 37b25fd88400df8cc726470b5d897111f2373b96 Mon Sep 17 00:00:00 2001 From: "Kali Kaneko (leap communications)" Date: Wed, 24 May 2017 14:22:26 +0200 Subject: [refactor] simplify wrapper create and add_msg - remove premature optimization for fast-notifies. blobs will cover that, no point in maintaning the optimization at the price of creeping complexity. --- src/leap/bitmask/mail/adaptors/soledad.py | 90 +++++++------------------------ src/leap/bitmask/mail/imap/account.py | 4 +- src/leap/bitmask/mail/imap/mailbox.py | 32 ++--------- src/leap/bitmask/mail/incoming/service.py | 3 +- src/leap/bitmask/mail/interfaces.py | 2 +- src/leap/bitmask/mail/mail.py | 48 +++-------------- 6 files changed, 30 insertions(+), 149 deletions(-) (limited to 'src/leap/bitmask/mail') diff --git a/src/leap/bitmask/mail/adaptors/soledad.py b/src/leap/bitmask/mail/adaptors/soledad.py index 25e4ad31..56bca363 100644 --- a/src/leap/bitmask/mail/adaptors/soledad.py +++ b/src/leap/bitmask/mail/adaptors/soledad.py @@ -126,7 +126,6 @@ class SoledadDocumentWrapper(models.DocumentWrapper): """ assert self.doc_id is None, "This document already has a doc_id!" - print "FUTURE", self.future_doc_id try: if self.future_doc_id is None: newdoc = yield store.create_doc( @@ -504,92 +503,39 @@ class MessageWrapper(object): cdoc.set_future_doc_id(doc_id) @defer.inlineCallbacks - def create(self, store, notify_just_mdoc=False, pending_inserts_dict=None): + def create(self, store): """ Create all the parts for this message in the store. :param store: an instance of Soledad - :param notify_just_mdoc: - if set to True, this method will return *only* the deferred - corresponding to the creation of the meta-message document. - Be warned that in that case there will be no record of failures - when creating the other part-documents. - - Otherwise, this method will return a deferred that will wait for - the creation of all the part documents. - - Setting this flag to True is mostly a convenient workaround for the - fact that massive serial appends will take too much time, and in - most of the cases the MUA will only switch to the mailbox where the - appends have happened after a certain time, which in most of the - times will be enough to have all the queued insert operations - finished. - :type notify_just_mdoc: bool - :param pending_inserts_dict: - a dictionary with the pending inserts ids. - :type pending_inserts_dict: dict - - :return: a deferred whose callback will be called when either all the - part documents have been written, or just the metamsg-doc, - depending on the value of the notify_just_mdoc flag + :return: a deferred whose callback will be called when all the + part documents have been written. :rtype: defer.Deferred """ - if pending_inserts_dict is None: - pending_inserts_dict = {} - assert self.cdocs, "Need cdocs to create the MessageWrapper docs" assert self.mdoc.doc_id is None, "Cannot create: mdoc has a doc_id" assert self.fdoc.doc_id is None, "Cannot create: fdoc has a doc_id" - def maybe_unblock_pending(): - if pending_inserts_dict: - ci_headers = lowerdict(self.hdoc.headers) - msgid = ci_headers.get('message-id', None) - try: - d = pending_inserts_dict[msgid] - d.callback(msgid) - except KeyError: - pass - copy = self._is_copy - try: - mdoc = yield self.mdoc.create(store, is_copy=copy) - print "GOT MDOC >>>>>>>>>>>>>>>>>>", mdoc, "copy?", copy - assert mdoc - self.mdoc = mdoc - except Exception: - self.log.failure('Error creating mdoc') - - if notify_just_mdoc: - # fire and forget, fast notifies - self.fdoc.create(store, is_copy=copy) - if not copy: - if self.hdoc.doc_id is None: - self.hdoc.create(store) - for cdoc in self.cdocs.values(): - if cdoc.doc_id is not None: - continue - cdoc.create(store) - - else: - yield self.fdoc.create(store, is_copy=copy) - if not copy: - if self.hdoc.doc_id is None: - yield self.hdoc.create(store) - for cdoc in self.cdocs.values(): - if cdoc.doc_id is not None: - # we could be just linking to an existing - # content-doc. - continue - yield cdoc.create(store) - - maybe_unblock_pending() + mdoc = yield self.mdoc.create(store, is_copy=copy) + assert mdoc + self.mdoc = mdoc + fdoc = yield self.fdoc.create(store, is_copy=copy) + self.fdoc = fdoc + if not copy: + if self.hdoc.doc_id is None: + hdoc = yield self.hdoc.create(store) + self.hdoc = hdoc + for cdoc in self.cdocs.values(): + if cdoc.doc_id is not None: + # we could be just linking to an existing + # content-doc. + continue + yield cdoc.create(store) defer.returnValue(self) - - def update(self, store): """ Update the only mutable parts, which are within the flags document. diff --git a/src/leap/bitmask/mail/imap/account.py b/src/leap/bitmask/mail/imap/account.py index 9b26dbaa..dab8812d 100644 --- a/src/leap/bitmask/mail/imap/account.py +++ b/src/leap/bitmask/mail/imap/account.py @@ -156,9 +156,7 @@ class IMAPAccount(object): """ name = normalize_mailbox(name) - # FIXME --- return failure instead of AssertionError - # See AccountTestCase... - leap_assert(name, "Need a mailbox name to create a mailbox") + assert name, "Need a mailbox name to create a mailbox" def check_it_does_not_exist(mailboxes): if name in mailboxes: diff --git a/src/leap/bitmask/mail/imap/mailbox.py b/src/leap/bitmask/mail/imap/mailbox.py index 0820a040..20b18761 100644 --- a/src/leap/bitmask/mail/imap/mailbox.py +++ b/src/leap/bitmask/mail/imap/mailbox.py @@ -317,7 +317,7 @@ class IMAPMailbox(object): d.addCallback(as_a_dict) return d - def addMessage(self, message, flags, date=None, notify_just_mdoc=True): + def addMessage(self, message, flags, date=None): """ Adds a message to this mailbox. @@ -330,24 +330,6 @@ class IMAPMailbox(object): :param date: timestamp :type date: str, or None - :param notify_just_mdoc: - boolean passed to the wrapper.create method, to indicate whether - we're insterested in being notified right after the mdoc has been - written (as it's the first doc to be written, and quite small, this - is faster, though potentially unsafe). - Setting it to True improves a *lot* the responsiveness of the - APPENDS: we just need to be notified when the mdoc is saved, and - let's just expect that the other parts are doing just fine. This - will not catch any errors when the inserts of the other parts - fail, but on the other hand allows us to return very quickly, - which seems a good compromise given that we have to serialize the - appends. - However, some operations like the saving of drafts need to wait for - all the parts to be saved, so if some heuristics are met down in - the call chain a Draft message will unconditionally set this flag - to False, and therefore ignoring the setting of this flag here. - :type notify_just_mdoc: bool - :return: a deferred that will be triggered with the UID of the added message. """ @@ -355,14 +337,7 @@ class IMAPMailbox(object): # TODO have a look at the cases for internal date in the rfc # XXX we could treat the message as an IMessage from here - # TODO change notify_just_mdoc to something more meaningful, like - # fast_insert_notify? - - # TODO notify_just_mdoc *sometimes* make the append tests fail. - # have to find a better solution for this. A workaround could probably - # be to have a list of the ongoing deferreds related to append, so that - # we queue for later all the requests having to do with these. - + # TODO -- fast appends should be definitely solved by Blobs. # A better solution will probably involve implementing MULTIAPPEND # extension or patching imap server to support pipelining. @@ -380,8 +355,7 @@ class IMAPMailbox(object): if date is None: date = formatdate(time.time()) - d = self.collection.add_msg(message, flags, date=date, - notify_just_mdoc=notify_just_mdoc) + d = self.collection.add_msg(message, flags, date=date) d.addCallback(lambda message: message.get_uid()) d.addErrback( lambda failure: self.log.failure('Error while adding msg')) diff --git a/src/leap/bitmask/mail/incoming/service.py b/src/leap/bitmask/mail/incoming/service.py index f1f4ab22..6a5d7fe3 100644 --- a/src/leap/bitmask/mail/incoming/service.py +++ b/src/leap/bitmask/mail/incoming/service.py @@ -847,8 +847,7 @@ class IncomingMail(Service): # Cannot do fast notifies, otherwise fucks with pixelated. d = self._inbox_collection.add_msg( - raw_data, (self.RECENT_FLAG,), date=insertion_date, - notify_just_mdoc=False) + raw_data, (self.RECENT_FLAG,), date=insertion_date) d.addCallbacks(msgSavedCallback, self._errback) return d diff --git a/src/leap/bitmask/mail/interfaces.py b/src/leap/bitmask/mail/interfaces.py index 10f51237..9ae61690 100644 --- a/src/leap/bitmask/mail/interfaces.py +++ b/src/leap/bitmask/mail/interfaces.py @@ -36,7 +36,7 @@ class IMessageWrapper(Interface): '(immutable)') cdocs = Attribute('A dictionary with the content-docs, one-indexed') - def create(self, store, notify_just_mdoc=False, pending_inserts_dict={}): + def create(self, store): """ Create the underlying wrapper. """ diff --git a/src/leap/bitmask/mail/mail.py b/src/leap/bitmask/mail/mail.py index 3dbb0a1f..9a7b1aed 100644 --- a/src/leap/bitmask/mail/mail.py +++ b/src/leap/bitmask/mail/mail.py @@ -41,7 +41,6 @@ from leap.bitmask.mail.constants import MessageFlags from leap.bitmask.mail.mailbox_indexer import MailboxIndexer from leap.bitmask.mail.plugins import soledad_sync_hooks from leap.bitmask.mail.utils import find_charset, CaseInsensitiveDict -from leap.bitmask.mail.utils import lowerdict log = Logger() @@ -89,6 +88,7 @@ def _encode_payload(payload, ctype=""): # soledad when it's creating the documents. # if not charset: # charset = get_email_charset(payload) + # TODO there's also some charset detection in the pixelated adapters. # ----------------------------------------------------- if not charset: @@ -368,8 +368,6 @@ class MessageCollection(object): store = None messageklass = Message - _pending_inserts = dict() - def __init__(self, adaptor, store, mbox_indexer=None, mbox_wrapper=None): """ Constructor for a MessageCollection. @@ -472,16 +470,7 @@ class MessageCollection(object): self.messageklass, self.store, doc_id, uid=uid, get_cdocs=get_cdocs) - def cleanup_and_get_doc_after_pending_insert(result): - for key in result: - self._pending_inserts.pop(key, None) - return get_doc_fun(self.mbox_uuid, uid) - - if not self._pending_inserts: - d = get_doc_fun(self.mbox_uuid, uid) - else: - d = defer.gatherResults(self._pending_inserts.values()) - d.addCallback(cleanup_and_get_doc_after_pending_insert) + d = get_doc_fun(self.mbox_uuid, uid) d.addCallback(get_msg_from_mdoc_id) return d @@ -579,8 +568,7 @@ class MessageCollection(object): # Manipulate messages @defer.inlineCallbacks - def add_msg(self, raw_msg, flags=tuple(), tags=tuple(), date="", - notify_just_mdoc=False): + def add_msg(self, raw_msg, flags=tuple(), tags=tuple(), date=""): """ Add a message to this collection. @@ -593,16 +581,6 @@ class MessageCollection(object): and time in the RFC-822 header, but rather a date and time that reflects when the message was received. :type date: str - :param notify_just_mdoc: - boolean passed to the wrapper.create method, to indicate whether - we're insterested in being notified right after the mdoc has been - written (as it's the first doc to be written, and quite small, this - is faster, though potentially unsafe), or on the contrary we want - to wait untill all the parts have been written. - Used by the imap mailbox implementation to get faster responses. - This will be ignored (and set to False) if a heuristic for a Draft - message is met, which currently is a specific mozilla header. - :type notify_just_mdoc: bool :returns: a deferred that will fire with a Message when this is inserted. @@ -610,24 +588,14 @@ class MessageCollection(object): """ # TODO watch out if the use of this method in IMAP COPY/APPEND is # passing the right date. - # XXX mdoc ref is a leaky abstraction here. generalize. + # XXX mdoc ref is a leaky abstraction here. generalize, that SHOULD be + # moved inside soledad adaptor. leap_assert_type(flags, tuple) leap_assert_type(date, str) msg = self.adaptor.get_msg_from_string(Message, raw_msg) wrapper = msg.get_wrapper() - headers = lowerdict(msg.get_headers()) - moz_draft_hdr = "X-Mozilla-Draft-Info" - if moz_draft_hdr.lower() in headers: - self.log.debug('Setting fast notify to False, Draft detected') - notify_just_mdoc = False - - if notify_just_mdoc: - msgid = headers.get('message-id') - if msgid: - self._pending_inserts[msgid] = defer.Deferred() - if not self.is_mailbox_collection(): raise NotImplementedError() @@ -639,11 +607,7 @@ class MessageCollection(object): wrapper.set_date(date) try: - updated_wrapper = yield wrapper.create( - self.store, - notify_just_mdoc=notify_just_mdoc, - pending_inserts_dict=self._pending_inserts) - + updated_wrapper = yield wrapper.create(self.store) doc_id = updated_wrapper.mdoc.doc_id if not doc_id: doc_id = updated_wrapper.mdoc.future_doc_id -- cgit v1.2.3