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.
After some discussion, we're okay considering that if CDS is down, all
of Signal is down. This will help protect session state and reduce the
risk of divergence.
Also changed the FeatureFlag guarding this code to
useOnlyModernContactDiscovery.
- Removed static UUIDBackfillTask initializer and removed the
registration from AppDelegate. Kicking off the task will be handled
with IOS-631.
- Moved the async perform work to its own function
- Some minor syntactic changes
- Updated database fetch to retrieve empty string UUIDs
- Handles non-e164 numbers fetched from database
- Leverages the MockSSKEnvironment to deal with the shared
SignalServiceAddressCache. Removes added test support.
- Add tests to exercise new functionality
This adds a whole bunch of tests for UUIDBackfillTask. This required a
small change to be made in SignalServiceAddress. Currently,
SignalServiceAddress accesses a singleton cache to force updates to any
existing addresses after a UUID is discovered. This breaks a whole bunch
of test verification.
To workaround this, I've added a testing_ flag to modify the singleton
SignalServiceAddressCache to temporarily disable it. This is going to
cause problems if we ever enable test parallelization, but it's all we
can do for now.
This also adds in incremental backoff logic, changes some of the log
statements and disables the task on production.
Also, adopted some minor feedback Matthew provided in the draft PR,
including:
- Rename: {signal -> registered}RecipientsWithoutUUID
- Move to owsFailDebug instead of assertionFailure
- Adopt `IsNetworkConnectivityFailure` for discerning network failures
- Add some extra assertions around our registered/unregistered sets
Introduces UUIDBackfillTask, which:
- fetches UUID-less SignalRecipients from the database
- creates a ContactDiscoveryOperation to lookup their UUIDs
- updates database with the UUID if available, unregisters otherwise
This is kicked off on an AppReadiness block submitted during
appDidFinishLaunching:
