Nora suggested that we also explicitly mark unregistered recipients as
unregistered in response to the message send preparation
ContactDiscoveryOperation.
Also, she found an opportunistic fix to remove the TSInfoMessage on
unregistered recipient failures. This isn't the behavior we want anyway.
Changes when we perform a CDS lookup for any phone-number-only
recipients. Now, we do the CDS lookup in a preparation phase prior to
performing the message send.
This also moves some of the error handling within the individual
OWSMessageSend handlers. Now, unregistered recipient handling is
factored out into its own method. _sendMessageToRecipient: will
considered the recipient unregistered if it's missing a UUID.
This adds a new async operation in the message send flow. MessageSender
will now verify that the intended recipients of a message all have UUIDs
before proceeding. If any recipients are missing a UUID, it'll kick off
a CDS lookup for the invalid recipients.
While this code appears to work, this is a first pass. Looking to get
some inital feedback in a pull request. It's unclear to me whether or
not this pattern fits in with the existing error propogation flow in
MessagSender promises. It's also unclear whether or not the error I'm
using is correct.
Some other changes that come along for the ride:
- Add a -perform() method to ContactDiscoveryService to automatically
submit itself and its dependencies to its static operation queue
- Matthew suggested moving all polite readiness blocks to their
impolite variants. There shouldn't be ordering concerns and message
processing is important enough to be run impolitely.
- Nora suggested moving some of the message processing checks into
MessagePipelineSupervisor. Some of the more complex checks like
registration are out of scope of this change. But -[AppContext
shouldProcessIncomingMessages] is a minor enough change to include.
Now, -shouldProcessIncomingMessages is a precondition for
-isMessageProcessingPermitted
- Matthew suggested that GRDB failures should be fatal. So instead,
AnySignalRecipientFinder will try! instead of try?.
- Matthew suggested moving some of the MessagePipelineSupervisor
logging statements to be lower priority.
- Adopt a local dependency property to fit in with existing style
- Unregister from pipeline supervision in some pipeline stages. This
usually shouldn't matter since they're all singletons/children of
singletons. I've only added this to classes that are already
unregistering existing observations, or don't have any existing
observations. Anything that was registering and not unregistering,
I've left as is.
Creates a new root singleton: OWSMessagePipelineSupervisor
As of right now, this singleton has two responsibilities:
- Track all message processing pipeline stages that have registered
themselves
- Post processing suspension updates to interested stages
Four classes will now register themselves as pipeline stages:
- OWSMessageContentQueue
- YAPDBMessageDecryptQueue
- IncomingGroupsV2MessageQueue
- SSKMessageDecryptJobQueue
At initialization, OWSMessagePipelineSupervisor will take out a pipeline
suspension while it waits for a UUIDBackfillTask to complete. Other
interested objects are also able to suspend the message processing
pipeline by invoking suspendMessageProcessing(for:) on the supervisor
(though, currently this is not used by anything except tests)
This also adds some supporting improvements to UnfairLock. Now,
UnfairLock closures will return the value returned from its critical
section closure.
