[Review] SF-0011: Concurrency-Safe Notifications

[kean]

Is there an alternative that doesn't introduce MainActorMessage and AsyncMessage? The former should probably be modeled using @MainActor or other existing language features.

The subject doesn't seem to require to share isolation with the message. Should it?

It's considered a best practice to call addObserver from the same thread on which the messages are sent from to make sure you don't miss any. Does addObserver enforce it or should it?

Does it support global actors? It's a non-starter if not. It seems that what it needs is a way to (automatically) inherit isolation from the message.

Does it need to be an extension of the existing NotificationCenter? It doesn't seem to share anything with this type except for the general idea of what it's for.

re: ObservationToken – it would be fantastic if this primitive was introduced on a standard library level.

[Philippe_Hausler]

@kean

ObservationToken – it would be fantastic if this primitive was introduced on a standard library level.

The implementation of that is just an Int64 under the hood - nothing special.

[Nickolas_Pohilets (Nickolas Pokhylets)]

So the post(_:, with:) has a parameter to for the subject, but it is not passed to makeNotification()? How does subject end up in Notification.object? Does method post(_:, with:) set it after calling makeNotification()?

If developers want subject to be part of the message, then makeMessage() is expected to parse it from the Notification, but makeNotification() is not expected to set it? This asymmetry is confusing.

If developers want subject to be part of the message, does that mean that when calling post it will look like post(Message(subject: subject), with: subject) (specifying subject twice)?

---

• The use of an isolated parameter enables the checking of an Actor-conforming type, but not the specific instance of an isolation, making it possible to post() with the wrong isolation. assertIsolated()/assumeIsolated() could be used to mitigate this behavior somewhat.

Nothing this proposal can do about this, but I'm really looking forward for support for dependent types for isolation.

[dnadoba (David Nadoba)]

I think ObservationToken should know to which NotificationCenter it belongs. Otherwise there is a chance that users can try to deregister from an unrelated NotificationCenter. Putting that on the users seems like a bad idea.
There should be a method on ObservationToken to stop observing it.

@cthielen

Should ObservationToken be non-copyable?
No, we don't need to limit the storage of these tokens. There's nothing in NotificationCenter that requires we limit how they are stored and copied.

I think we should still make it non-copyable and make it automatically deregister on deinit.

[FranzBusch]

@cthielen

When post() is called, the default behavior is for the observer closures to be executed without hopping threads. So, if a framework vends a MainActorMessage, it's expected that the observer closure is executed on the main actor because the post() that ultimately triggers the closure was called from main actor.

Right, that's the synchronous delivery that the current NotificationCenter APIs guarantee and utmost important to uphold ordering and prohibit data races. However, I would argue that this is broader than just notifications related to MainActor state. There are probably many applications out there build around NotificationCenter that expect synchronous delivery for non-MainActor related messages.

@cthielen

If I understand correctly, if we allowed a MainActorMessage observer to execute off the main actor, there'd be a mismatch between what was occurring at runtime (they are indeed on the main actor by way of the post()) and what the compiler determined at compile-time, e.g. it would think you're in a different isolation with a Sendable, synchronous closure.

I never proposed that. MainActorMessages must always be posted on the MainActor; however, if we marked the protocol as @MainActor we would also allow to observe them off the MainActor. Though accessing them would require awaits. From a data race perspective this is sound. The question is: Is this useful?

@cthielen

My understanding is that @MainActor on a type only has an effect on where its methods are called. I think you may be right that it won't be common for a developer to construct these types off the main actor (and subsequently use one of its methods), but does that mean we should limit such type methods to only run on @MainActor in scenarios where that may not be the case?

I am also not sure what the right answer is but I think it at least needs some explanation in the proposal why we think going either way is the right one. The current approach is totally fine but prohibits off MainActor observing off MainActorMessages.

@cthielen

Your performance concern is a good one, and we need to update the proposal to document this aspect: NotificationCenter maintains an arbitrary isolation that it uses to call all AsyncMessage observers in serial, so we're able to call these observers without the expense of an unstructured task per observer call.

Thanks for spelling this out. I personally don't think that's the right approach. We shouldn't spawn an unstructured task, even if it's only one, in such a low level primitive type. Rather we should express this using the structured tools that the language offers.

Moreover, I am surprised that the new APIs would serialise all the messages for me and most likely call me on a different thread than the one the messages was posted on.

@cthielen

AsyncMessage is designed for cases where the post() is on a background thread or other isolation. We internally execute the observer within an async context but do not guarantee which isolation it is. This matches the behavior today where a Notification observer runs on an arbitrary background thread, but now AsyncMessage makes this contract explicit, requiring the developer to await if they need to hop back to MainActor or another isolation.

Does it really match the behaviour from the current Notification? Right now you can synchronously observe any notification that is posted to a centre by specifying queue: nil when adding an observer. In my opinion, this proposal tries to solve two things at once:

• Introducing a way of differentiating between MainActor isolated messages and messages that can be posted on any isolation context
• Allowing asynchronous observers

I would argue the second one is a non-goal for three reasons:

1. It requires you to spawn an unstructured task and serialise all observer calls
2. It disallows synchronously observing AsyncMessages
3. The current .notifications() APIs already allow asynchronous serialised observation

@cthielen

I'm curious if there's another use case you have in mind where an AsyncMessage should be able to be called synchronously, or if the concern is that there isn't a way using MainActorMessage nor AsyncMessage to synchronously call observers that are arbitrarily isolated?

Yes, I strongly believe there are many use-cases where synchronous delivery of non-MainActor messages is utmost important and we would completely eliminate that use-case with this proposal.

Taking a step back. My understanding is that the biggest problem that this proposal tries to solve is the current Sendable problems around object and userInfo taking Anys which means we can never make the current Notification type Sendable. Even if we could do that some notifications as you pointed out are inherently tied to some isolation e.g. the MainActor.

I think this proposal does a good job of introducing a new abstraction i.e. Messages and the MainActorMessage protocol to express Concurrency-Safe notification types. That's all really great.

The only larger thing that I personally disagree with is forcing all non-MainActorMessage users into asynchronous observation. Marking the addObserver closure as @Sendable and offering an AsyncSequence based APIs should be enough.

[gwendal.roue (Gwendal Roué)]

@FranzBusch

The only larger thing that I personally disagree with is forcing all non-MainActorMessage users into asynchronous observation. Marking the addObserver closure as @Sendable and offering an AsyncSequence based APIs should be enough.

2 × 2 cents:

• I think I fully agree with @FranzBusch: NotificationCenter should not create Tasks, and async notification handlers looks out of place since synchronous delivery of notifications has always been 1. a very important feature of NotificationCenter and 2. a use case that still exists, even out of the main actor).

• I remember an important forum thread (Reliably testing code that adopts Swift Concurrency?) that was using async sequences built from NotificationCenter as an example of an api that was difficult to test (and generally to use) because the task that starts iterating notifications can easily miss the first ones. Call this a data race of a race condition, I don't quite care: this is a bug-prone api. I wish this problem was fully acknowledged, and very explicitly considered and addressed, if not solved, in this proposal.

[cthielen (Christopher Thielen)]

Thanks again everyone for the feedback. Continuing a few of the items that were brought up:

AsyncMessage and Synchronous, Non-MainActor Posting
It's true that Message & Sendable is enough to have an observer run synchronously on the thread of the poster, but this isn't too different from the status quo today: addObserver() with Notification will run an observer in a synchronous, non-isolated context, requiring a developer who wishes to mutate state in response to a Notification to either only interact with other non-isolated variables, or spin up a Task to mutate state. For messages and observers that run on MainActor, MainActorMessage is a good solution. For background posters, it's a little trickier.

We're constrained by existing uses of post() which may be on a background thread and which expect post() to be a synchronous function, looping through all observers synchronously to call them.

To account for this, we synchronously enqueue the observers to be processed by a single Task, maintained by NotificationCenter. This results in post() remaining synchronous, while the Task can run each observer.

If we inherited the isolation of the observer declaration site, we would need a way to enqueue the observer call onto the observer declaration site's isolation synchronously (to ensure post() remains synchronous), or we would need to continue to use our existing Task solution, but make a second isolation hop onto the observer declaration site's isolation.

makeNotification()/makeMessage() and Subject
Message and Notification are not quite equivalent to each other - while Notification packages userInfo and object together, many use cases of Notification have a nil object, or only ever use object to filter notifications, but don't otherwise use the object in other logic. This is one of the reasons we've designed Message to consider Subject as something separate from the Message. Developers are free to re-use Subject in Message if it makes sense.

At the moment, makeNotification() is not expected to fill in notification.object, opting instead to favor the subject passed in to post(). We'll have to consider this behavior further, particularly for the use cases where a developer is ferrying their subject within Message. Perhaps it would make more sense to have makeNotification(from: message, with: subject) but keep makeMessage(from: notification)?

MainActorMessage vs @MainActor
@MainActor does not participate in Swift generics, so we're unable to offer an overload of addObserver() based on a type with this annotation alone.

ObservationToken and non-copyable
If ObservationToken was non-copyable, we would likely want its deinit to remove the observer registration, but that would mean letting the token go out of scope would result in observers being removed "implicitly", which could cause bugs. I think that unfortunate behavior may be worse than the benefit of having a non-copyable ObservationToken.

[dnadoba (David Nadoba)]

@cthielen

ObservationToken and non-copyable
If ObservationToken was non-copyable, we would likely want its deinit to remove the observer registration, but that would mean letting the token go out of scope would result in observers being removed "implicitly", which could cause bugs. I think that unfortunate behavior may be worse than the benefit of having a non-copyable ObservationToken.

You are right and this is a desirable behaviour as it prevents a memory leak! There are some case where you want to create an observer indefinitely but there are also a ton where you need to stop observing at some point.
We should by default optimize for the use case where we want to stop observing at some point.

We can also introduce a method on ObservationToken, called immortal(), .forget(), .forever() or similar that consumes self without actually stopping observing to support the indefinite use case without requiring any storage if we really want to.

[ahti (Lukas Stabe 🙃)]

@cthielen

ObservationToken and non-copyable
If ObservationToken was non-copyable, we would likely want its deinit to remove the observer registration, but that would mean letting the token go out of scope would result in observers being removed "implicitly", which could cause bugs. I think that unfortunate behavior may be worse than the benefit of having a non-copyable ObservationToken.

Just as a single anecdotal data point: Our app uses a ton of notifications to keep UI up to date with model state, and I don't recall a single place where we do not stop the observation in deinit of the observing object.

So stopping observation when the token is deinited would save us a fair bit of boilerplate, and I'd be all for this behaviour.

[FranzBusch]

Thanks for the reply @cthielen.

@cthielen

It's true that Message & Sendable is enough to have an observer run synchronously on the thread of the poster, but this isn't too different from the status quo today: addObserver() with Notification will run an observer in a synchronous, non-isolated context, requiring a developer who wishes to mutate state in response to a Notification to either only interact with other non-isolated variables, or spin up a Task to mutate state. For messages and observers that run on MainActor, MainActorMessage is a good solution. For background posters, it's a little trickier.

We're constrained by existing uses of post() which may be on a background thread and which expect post() to be a synchronous function, looping through all observers synchronously to call them.

To account for this, we synchronously enqueue the observers to be processed by a single Task, maintained by NotificationCenter. This results in post() remaining synchronous, while the Task can run each observer.

I want to reply to all of this at once. I think we are conflating several things at once here in the proposed solution. To me it looks like there are three distinct things, please let me know if you think there are other goals that we want to achieve.

1. Creating a safe API for the current Notification struct that can be Sendable

This is well addressed by the new Message protocol.

2. Allowing synchronous observation

This is only addressed by the MainActorMessage and the addObserver methods that are constrained to @MainActor. However, it is impossible for normal Messages or AsyncMessages. I still remain convinced that this is a mistake and we should allow synchronous observation of any Message & Sendable with an addObserver method that takes a synchronous @Sendable closure.

3. Allowing observation in the isolation domain of the observer

The proposal tries to address this by taking an async closure in the addObserver API for AsyncMessage. This allows to hop the isolation in the closure back to the one in the surrounding context. Importantly, the closure is also @Sendable which means the isolation is not automatically inherited. The closure is still nonisolated. As discussed above but just to restate, this requires us to spawn an unstructured Task in the NotificationCenter implementation to which we send the posted message and then call all the registered observers from. Importantly, this means messages are asynchronously delivered.

I still strongly believe this is the wrong thing to do. First, because we should not spawn any unstructured tasks in the standard library or Foundation. Secondly, because it seems unnecessary to serialise all observer calls.

Now I think the right solution for this problem is to just expose an AsyncSequence API like the .notifications() on NotificationCenter already does. This already allows observes to consume them in their isolation domain.

@cthielen

If we inherited the isolation of the observer declaration site, we would need a way to enqueue the observer call onto the observer declaration site's isolation synchronously (to ensure post() remains synchronous), or we would need to continue to use our existing Task solution, but make a second isolation hop onto the observer declaration site's isolation.

I just want to emphasis my feedback on this particular point again. There is no way to achieve both synchronous delivery of messages while delivering the message to the isolation of the observer. That's just not possible unless we get new language features to express dependent types for isolation.
For non-MainActor observation, we can either allow synchronous observation via closure that's sync @Sendable or asynchronous observation via an AsyncSequence. The latter can easily be modelled and implemented via the former.

[tclementdev]

Just want to say +1 with everything @FranzBusch just said. Especially synchronous observation via a @Sendable closure, it's critical enough that I think it should just be part of the story from day one.

[Nickolas_Pohilets (Nickolas Pokhylets)]

@cthielen

At the moment, makeNotification() is not expected to fill in notification.object, opting instead to favor the subject passed in to post(). We'll have to consider this behavior further, particularly for the use cases where a developer is ferrying their subject within Message. Perhaps it would make more sense to have makeNotification(from: message, with: subject) but keep makeMessage(from: notification)?

Let's assume that Message replaces name and userInfo, but not object. Then makeNotification() should be returning only name and userInfo, maybe as a new type. And actually there is already a separate static property for name and makeNotification() cannot really return any different name. So we don't need to return name at all. All we are left with is userInfo. Which leads me to conclusion that makeNotification() should be replaced with static func getUserInfo(_ message: Self) -> [AnyHashable: Any] or even var useInfo: [AnyHashable: Any].

And to makes things clear, we can also change makeMessage, since name is known statically and object should be ignored.

static func makeMessage(_ userInfo: [AnyHashable: Any]) -> Self

Typing [AnyHashable: Any] is a bit too verbose, but a simple typealias Notification.UserInfo = [AnyHashable: Any] would help.

But now we have a problem - in use cases when one subscribes listens all senders, subscriber may need to know which sends posted the notification. To accommodate for this, I suggest to add extra parameter to the overload listening multiple senders:

public func addObserver<I: MessageIdentifier, M: MainActorMessage>(
    of subject: M.Subject.Type,
    for identifier: I,
    using observer: @escaping @MainActor (M.Subject, M) -> Void
) -> ObservationToken where I.MessageType == M

And maybe this warrants splitting overload taking M.Type into two:

public func addObserver<M: MainActorMessage>(
_ messageType: M.Type = M.self,
subject: M.Subject,
using observer: @escaping @MainActor (M) -> Void
) -> ObservationToken

public func addObserver<M: MainActorMessage>(
_ messageType: M.Type = M.self,
subject: M.Subject.Type,
using observer: @escaping @MainActor (M.Subject, M) -> Void
) -> ObservationToken

Also we could add a default value for messageType for cases when M can be inferred from the closure.

But for subject: M.Subject.Type I would strongly prefer to have an explicit parameter. IMO, this is an anti-pattern. It does exist in the wild, and probably still should be supported, but not encouraged.

[cthielen (Christopher Thielen)]

When thinking about NotificationCenter and Swift Concurrency, we've found it helpful to visualize this table:

              |   Sync Delivery   | Async Delivery
--------------------------------------------------
    MainActor |  MainActorMessage |  AsyncMessage
------------- | ----------------- | --------------
Non-MainActor |        ???        |  AsyncMessage

It sounds like there are two sticking points here: synchronous observation for non-MainActor use cases and how asynchronous observation of messages should work.

Synchronous, Non-MainActor Observation
We understand the importance of this - it's something NotificationCenter does today - but we're still thinking through a solution to this, and in the interim, are presenting this proposal which handles the other cases of synchronous, MainActor-bound observation, and asynchronous observation. We expect this will solve the majority of existing NotificataionCenter concurrency concerns today (a lot of existing Notifications are MainActor), but will continue working on a solution for synchronous, non-MainActor observation and may post a follow-up proposal addressing this. We recognize this is an important capability.

One of our concerns with providing a version of addObserver() that uses Sendable, synchronous closures that are either nonisolated or have unknown isolations (e.g. post() called from Objective-C on some background thread), is that we would be providing an API with similar ergonomic issues to the ones we're trying to address with this proposal.

Asynchronous Observation of Messages
NotificationCenter today delivers synchronously, either on the poster's thread or executed using an OperationQueue. Our intention with AsyncMessage is to solve some of the non-MainActor use cases - those which do not need synchronous/transactional timing and may benefit from having an asynchronous context. (Note that the choice of MainActorMessage vs. AsyncMessage is determined by the framework author, not the user.)

AsyncMessage provides an ergonomic convenience to developers that discourages the practice of spinning up their own unstructured task per observation (which is very tempting from an otherwise non-isolated, synchronous context), and it does so at a cost similar to today's OperationQueue. The single Task per NotificationCenter instance is an implementation detail and subject to change in the future. Its serial nature matches the behavior of NotificationCenter today, but there is no reason AsyncMessages must be processed serially.

The idea of an AsyncStream or AsyncSequence of AsyncMessage is an interesting one which may be valuable to provide, and we may explore that in a followup proposal.

[icharleshu (Charles Hu)]

Thanks everyone for your comments! We received good participation from the community -- feedback was mixed: bringing stronger typing to notifications was well-received, but there were a few concerns and suggestions:

• The inability to express synchronous, non-MainActor observers.
• Consider vending AsyncSeqeunce-style ovservation instead of closures
• Consdier making ObservationToken non-copyable so observable can be cancelled in deinit.
• whether MainActorMessage should be marked as @MainActor.

The authors addressed these concerns separately and aknowledge that this proposal does not support synchronous observation off the non-MainActor and therefore does not offer a one-for-one replacement for the existing Notification API. I agree with the aurthors that the scope of this proposal is to solve the majority of existing NotificataionCenter concurrency concerns today rather than a boarder overhall of NotificationCenter. Therefore we should't hold up these improvements while we consider the best strategy to implement synchronous non-main actor observers. Swift Concurrency and related design patterns continue to evolve, and we may revisit this missing functionality in near future.

This proposal is accepted if the authors make the following modification: given the missing important use cases stated above, authours should update the "Future Directions" of this proposal to clarify how could the current API evolve to support these features in near future.

[dnadoba (David Nadoba)]

@icharleshu

Consdier making ObservationToken non-copyable so observable can be cancelled in deinit.

@icharleshu

The authors addressed these concerns separately

Concerns about confusing ObservationTokens from different NotificaitonCenter instances in removeObserver(_:) hasn't been addressed.

In addition, the latest concern about removeObserver(_:) wasn't addressed by the author. ObservationToken as it is proposed is close to manual memory management with the exception that it can't be double freed (I hope). It would be great if the need for manual memory management could be explained and an automatically managed version be included. Non-copyable is just one possible solution that doesn't require an allocation and therefore doesn't have a perf impact. Classes are totally fine too, just with a perf hit. Putting manual memory management onto a user should need to be motivated by a clear use case and be an opt-out rather than be the default for a high level API like NotificationCenter.

The proposal still uses non standard abbreviation instead of names for generic parameters and should be avoided according to the design guidelines:

Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.

The intended meaning for any abbreviation you use should be easily found by a web search.

Swift.org - API Design Guidelines

(nit: the code formatting of the proposal is also still inconsistent and rather hard to read)

[Nickolas_Pohilets (Nickolas Pokhylets)]

@cthielen

Synchronous, Non-MainActor Observation
We understand the importance of this - it's something NotificationCenter does today - but we're still thinking through a solution to this, and in the interim, are presenting this proposal which handles the other cases of synchronous, MainActor-bound observation, and asynchronous observation. We expect this will solve the majority of existing NotificataionCenter concurrency concerns today (a lot of existing Notifications are MainActor), but will continue working on a solution for synchronous, non-MainActor observation and may post a follow-up proposal addressing this. We recognize this is an important capability.

One of our concerns with providing a version of addObserver() that uses Sendable, synchronous closures that are either nonisolated or have unknown isolations (e.g. post() called from Objective-C on some background thread), is that we would be providing an API with similar ergonomic issues to the ones we're trying to address with this proposal.

Swift already has a type system tool that kinda behaves as if it is generic over the isolation - non-sendable types. But I'm not exactly sure what instance should be used as a marker of the isolation.

My first thought was that Sender could be used for that. But since Sender is specified by the user of the framework, and there is no way to force this type to be non-sendable, this is not reliable. And, of course, sender is optional in the API.

So what about notification center itself? NotificationCenter is already Sendable, but we can still create a non-sendable view for a specific actor:

extension NotificationCenter {
    // Returns the same instance for the same argument
    // Maintains [WeakRef<AnyActor>: IsolatedNotificationCenter] under the hood.
    func isolated(to isolation: AnyActor) -> IsolatedNotificationCenter
}

class IsolatedNotificationCenter /* not sendable! */ {
    public func post<M: Message>(_ message: M, with subject: M.Subject)
    public func post<M: Message>(_ message: M, with subject: M.Subject.Type)

    public func addObserver<M: ???>(
        _ messageType: M.Type,
        subject: M.Subject? = nil,
        using observer: @escaping (M) -> Void // Note that closure is also not sendable
    )  -> ObservationToken
}

If messages are posted on one actor, but observed on another - they are not delivered. And there is no way to associate specific actor with the message type.