AsynchronousOperation thread safety issue

[lilyball]

AsynchronousOperation is used in contexts where it may be completed from arbitrary background threads, but its state property is not thread-safe. In practice this is likely to work ok, because OperationQueue will receive and react to KVO notifications from the thread that mutated the state property, but there's always a risk where if OperationQueue checks e.g. isFinished on the operation at the same time that the state property is mutated on a background thread then it will violate the threading model and may read a state value that was never written.

Unfortunately Swift doesn't have access to atomic values, so I'm not quite sure what the best solution here is. If Apollo was a hybrid library I'd say this class should simply be written in Obj-C and use C11 atomics. There are a number of options for pure Swift but they're all going to be much slower than proper atomics. Possibly the best one is to use a pthread_mutex_t that's explicitly configured with the PTHREAD_MUTEX_POLICY_FIRSTFIT_NP policy (as this is an order of magnitude faster than the default "fair" lock). You could also just use objc_sync_enter(self) and objc_sync_exit(self); these use a recursive mutex, so locking/unlocking will be slower, but the system will reuse the mutexes when the associated objects deallocate which means they may be a good choice for large numbers of short-lived objects.

[designatednerd]

@lilyball Have you seen this cause issues in practice? I can see where the theoretical issue lies, but I'm curious as to whether it's actively causing issues or is more of a "This may someday blow up in our faces and we should be aware of that"

[lilyball]

This may someday blow up in our faces and we should be aware of that.

Or more accurately, this will likely be a source of very infrequent bad behavior or crashes, which is hard to diagnose and debug, but may possibly show up in high-usage apps.

[designatednerd]

Yeah, fair point. There's definitely a lot of stuff where we need to get some much better practices in here, I'm just trying to figure out where to apply the duct tape first 😄

[designatednerd]

"Fixing" this by removing the class altogether in the updated networking stack being put together in #1341. Gonna close this one out, sorry we were never able to untangle it!