Skip to content

Instantly share code, notes, and snippets.

@siracusa
Last active June 17, 2024 16:47
Show Gist options
  • Save siracusa/137e3fae8a384ac4bda8c56524826d6b to your computer and use it in GitHub Desktop.
Save siracusa/137e3fae8a384ac4bda8c56524826d6b to your computer and use it in GitHub Desktop.
Swift Concurrency Candidate
NSWorkspace.shared.notificationCenter.addObserver(
forName: NSWorkspace.didLaunchApplicationNotification,
object: nil, queue: nil, using: { [weak self] notification in
self?.doStuff()
})
@siracusa
Copy link
Author

Yeah, the docs for that method say (under the queue parameter, which is maybe not where I'd first look for it…)

The operation queue where the block runs.
When nil, the block runs synchronously on the posting thread.

So if I call it in applicationDidFinishLaunching (which I assume is running on the main thread) and pass a nil queue, then it should work. (So should .main, I guess.)

@mattmassicotte
Copy link

The docs specifically say nil will cause the notifcation center to be delivered on the posting thread. And it is not documented how NSWorkspace does its internal work, so this code could actually be incorrect without the explicit .main. But I do not know for sure.

And this is really reminding me of the show you did with Holly and Ben. The compiler is forcing you here to deal with NotificationCenter's ambigous API. It's annoying! It's probably fine! But, the only way to guarantee correctness here is to add more code above just some annotations.

I also want to be really clear that I deeply sympathize with the warnings being hard to understanding and unhelpful.

@danielpunkass
Copy link

danielpunkass commented Jun 16, 2024

I ran into similar issues with regular NotificationCenter, and tried to generalize a helper method that would let me subscribe to notifications on the main actor, and elide the warnings about the callback's @sendable nature.

Unfortunately I haven't been able to get the captures to work. Even if I wrap the callback in an assumeIsolated:

extension NotificationCenter {

   @MainActor @discardableResult
   public func addMainActorObserver(forName name: Notification.Name, object obj: Any? = nil, using block: @escaping @MainActor (Notification) -> Void) -> any NSObjectProtocol {
      return self.addObserver(forName: name, object: obj, queue: .main) { note in
        MainActor.assumeIsolated {
          block(note)
        }
      }
   }

}

The block(note) line generates errors like:

Capture of 'block' with non-sendable type '@MainActor (Notification) -> Void' in a `@Sendable` closure

I have experimented with adding @MainActor to the closure, but in any case it insists that the closure is @Sendable.

Any clues appreciated! Thanks.

@mattmassicotte
Copy link

Making what you want is very reasonable. But it is extremely hard because of how NotificationCenter actually works.

First, your implemenation actually makes sense! And it would have gotten you further, except for a dumb limitation. We were talking earlier about how when you add isolation to something (like putting on a MainActor), it implictly makes it Sendable. It give the compiler enough information to correctly deal with it at any access site.

It turns out that rule doesn't yet apply to closures. But, it will when SE-0434 becomes available!

So, workaround number one: the block type actually needs to be @escaping @MainActor @Sendable (Notification) -> Void. This artifical limit of the closure being @Sendable can make it much less useful. But it gets worse!

Because once you do that, you run into the real problem, which has been an issue for many developers: Notification is not, and can never be Sendable. So, you now have this problem:

addObserver(forName: name, object: obj, queue: .main) { note in
    MainActor.assumeIsolated {
        // ERROR: Sending 'note' risks causing data races
        block(note)
    }
}

I don't think there is any way to workaround this. Notification allows arbitrary references in both its object and userInfo properties.

Here's the best I can come up with, which is not excellent, but may give you an idea:

@MainActor
public func addMainActorObserver<Payload>(
    forName name: Notification.Name,
    object obj: Any? = nil,
    // We need to a way to transform the non-Sendable `Notification` into something we can use to cross boundaries
    process: @escaping @Sendable (Notification) -> (Payload),
    // The addition of both @MainActor  @Sendable is currently necessary until SE-0434 is integrated
    using block: @escaping @MainActor @Sendable (Payload) -> Void
) -> any NSObjectProtocol where Payload : Sendable {
    return self.addObserver(forName: name, object: obj, queue: .main) { note in
        let payload = process(note)

        // here is the boundary we have to cross
        MainActor.assumeIsolated {
            block(payload)
        }
    }
}

This stinks.

@danielpunkass
Copy link

Glad to know I’m not missing something obvious, but yeah this is an unfortunate limitation. I wonder if some kind of nasty Swift bitcasting, or an ObjC helper method might be a workaround. This pattern is common enough in my code base that eliminating the clutter of all those “yes this is the main actor” assumeIsolated calls is very attractive.

@danielpunkass
Copy link

I think I got something working. I implemented an ObjC helper:

- (id <NSObject>)_addMainQueueObserverForName:(nullable NSNotificationName)name object:(nullable id)obj usingBlock:(void (NS_SWIFT_UI_ACTOR ^)(NSNotification *notification))block
{
	return [self addObserverForName:name object:obj queue:NSOperationQueue.mainQueue usingBlock:^(NSNotification * _Nonnull notification) {
		block(notification);
	}];
}

But for whatever reason the NS_SWIFT_UI_ACTOR wasn't coming through to the Swift side? I'll try to figure out a fix for that. But in the meantime, wrapping the ObjC helper above with a Swift-facing call that just calls through to it:

@MainActor @discardableResult
public func addMainActorObserver(forName name: Notification.Name, object obj: Any? = nil, using block: @escaping @MainActor (Notification) -> Void) -> any NSObjectProtocol {
	return self._addMainQueueObserver(forName: name, object: obj, using: block)
}

Seems to work!

@mattmassicotte
Copy link

Oh well actually yes, you can achieve something similar in Swift:

extension NotificationCenter {
    @MainActor
    public func addMainActorObserver(
        forName name: Notification.Name,
        object obj: Any? = nil,
        block: @escaping @MainActor (Notification) -> Void
    ) -> any NSObjectProtocol {
        // this let's you lie to the compiler!
        let sendableBlock = unsafeBitCast(block, to: (@Sendable (Notification) -> Void).self)

        return self.addObserver(forName: name, object: obj, queue: .main, using: sendableBlock)
    }
}

I'm not sure exactly what's up with the NS_SWIFT_UI_ACTOR. I haven't looked into the clang annotations deeply but they are definitely supposed to work. Please file a bug if something isn't working as it should!

@danielpunkass
Copy link

Perfect, thanks for the bitcast hack! And I'll look into the NS_SWIFT_UI_ACTOR issue and file a bug if it's busted.

@danielpunkass
Copy link

Also for anybody reading along: I think I decided I don't want/need the addMainActorObserver to be @MainActor. It's likely I'll only ever call it from the main thread, but because the method guarantees delivery on the main thread, it can be safely called from anywhere, I think.

@mattmassicotte
Copy link

I think this is correct! But, I also do want to point out that this version makes it possible to pass data unsafely across threads by way of the Notification object. I don't think that's likely though.

@danielpunkass
Copy link

Oh, right. That's a good point. Maybe I should leave it @MainActor after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment