Instead of storing the event loop here I am wondering if we can just store a cancel closure. I think I tried that with ScheduledTask once but I am unsure if I got it down to 0 allocations. That might allow us to avoid the whole UInt64 completely

I will try this again, but my intuition is that this will not be as performant. Is avoiding the UInt64 a goal?

I tried this and it resulted in an allocation. I have kept the commits in the history so you can see the attempt and the revert:

• b3e4903: feedback(attempted): Store a closure instead of UInt64
• fb5e835: feedback(reverted, allocates): Store a closure instead of UInt64

We now have two places here that create a task id and then a ScheduledTask can we de-dupe the code into a shared method?

I wanted to keep the CustomTimerImplementation conformance in its own extension here for clarity but, yes, let me take a look and see what refactoring I can do in the SelectableEventLoop to provide a reusable method.

Similar here can we de-dupe this code

Is it necessary for us to add a new protocol? Any reason not to have EventLoop provide this directly?

Yes, I tried to outline this in the PR description, but the goal here is to have a simple default implementation and have this as an opt-in extension point for implementations that want to do something different (e.g. more performant).

If we elevated these to the EventLoop protocol how would the default implementation look? Additionally, the protocol refinement here allows the type system to enforce that an implementation that's opting in here implements both halves of the customisation point.

I don't think we ever want the default implementation: it's almost always worse than doing something bespoke. We have to provide it to avoid the API break, but we don't really care about it.

If we elevated these to the EventLoop protocol how would the default implementation look?

More or less exactly as it does now. cancel is straightforward to implement, as you already have.

Additionally, the protocol refinement here allows the type system to enforce that an implementation that's opting in here implements both halves of the customisation point.

This is handy, but doesn't necessarily outweigh the noise of the extra protocol.

More broadly, because we have added a new protocol, we make all event loops publicly offer these two methods:

    func setTimer(for deadline: NIODeadline, _ handler: any NIOTimerHandler) -> UInt64
    func cancelTimer(_ id: UInt64)

But we don't want these two methods, we want the two that return typed handles. Nonetheless, users will see and be able to call these two on event loops. I think that's a strong enough API argument to reject this spelling.

More broadly, because we have added a new protocol, we make all event loops publicly offer these two methods:

    func setTimer(for deadline: NIODeadline, _ handler: any NIOTimerHandler) -> UInt64
    func cancelTimer(_ id: UInt64)

But we don't want these two methods, we want the two that return typed handles. Nonetheless, users will see and be able to call these two on event loops. I think that's a strong enough API argument to reject this spelling.

Presumably it would be only event loops that conform to this protocol that would publicly expose these methods and only when they are being held as a concrete type, rather than an any EventLoop?

However, I do concede that these symbols end up being public when ideally they should be internal.

I did this because I thought that was what the design called for: a slow default and the ability to provide something faster.

Maybe I've been thinking about exposing the wrong half of this. In the current PR I'm exposing the new methods (as a new protocol with new methods) and keeping the typed handle very opaque. Are you suggesting that I make the typed NIOTimer less opaque. Specifically that users can construct it as either a scheduledTask or a custom (will think on a better name if it's going to be public) which would allow event loop implementations to still opt in to provide a more performant implementation?

OK, so I've gone ahead and made the patch that I believe is more in line with this feedback.

We no longer have another protocol, but instead we now have to be careful about ensuring we provide the right overloads for both the schedule and cancel functions and need a runtime crash if the contract has been violated.

IMO, this was a place we could have had the type system help us and any external authors of event loops.

I appreciate the argument that it would result in the exposing of lower level functionality to end users, but they don't have to call that, and, if they did it would be safe still.

Possibly a moot point now, but would you have considered a layered approach like that initially proposed if we made use of @_spi to expose it only to event loop implementors?

The targeted relevant changes for this feedback are in 89c57ec.

Possibly a moot point now, but would you have considered a layered approach like that initially proposed if we made use of @_spi to expose it only to event loop implementors?

I'm not convinced this would actually have worked. It's not immediately clear to me what the semantics of conformance to @_spi protocols actually is, so it's not clear to me that this would have prevented the protocol conformance being essentially public or that it would have successfully populated the witness table.

If we assume that it would work as intended, though, I'm still not sure I'd have gone for it. I feel less strongly: the extra API noise is removed. But it would be a bit challenging to communicate to implementors how they should provide this implementation. Ultimately, we'd have to document it: otherwise it won't occur to implementors that they should implement this other protocol to implement these two functions. We have a lot of protocols, and it'll be easy to miss.

Once we're relying on documentation for correct implementation, I don't know that it's much worse to simply document that implementors must provide both methods. Yes, the type system will help correct implementations, but incorrect implementations will fail almost immediately the moment anyone tries to use them, including the implementor. So I'm just fundamentally not convinced that the downside risk is worth the extra abstractions.

Let's add another bikeshed: is there any reason not to make this method generic?

Done in 06a4ce7. Although I didn't see much improvement from it.

While I'm here, can I suggest that we should use different labels instead of for in both? That will help with tab-completion a bit.

Hm, it felt OK to me. Would you prefer the following?

• setTimer(for: TimeAmount, ...)
• setTimer(forDeadline: NIODeadline, ...)

I'm thinking of how I set timers verbally on my phone I normally always use "set a timer for" regardless of whether I then say a duration or an absolute time.

Open to suggestions here, though :)

setTimer(for:) and setTimer(at:) is probably the easiest spelling distinction.

https://github.com/apple/swift-nio/pull/2759/files/df3cdb1c1f1f3ce8fb66fd81cdfa0cef3318c984#r1664313726 resulted in new API spellings. But these do use different prepositions for the NIODeadline and TimeAmount variants: scheduleCallback(at:handler:) and scheduleCallback(in:handler:), respectively.

https://github.com/apple/swift-nio/pull/2759/files/df3cdb1c1f1f3ce8fb66fd81cdfa0cef3318c984#r1664313726 resulted in new API spellings. But these do use different prepositions for the NIODeadline and TimeAmount variants: scheduleCallback(at:handler:) and scheduleCallback(in:handler:), respectively.

I tried this and it resulted in an allocation. I have kept the commits in the history so you can see the attempt and the revert:

• b3e4903: feedback(attempted): Store a closure instead of UInt64
• fb5e835: feedback(reverted, allocates): Store a closure instead of UInt64

Done in 06a4ce7. Although I didn't see much improvement from it.

OK, so I've gone ahead and made the patch that I believe is more in line with this feedback.

We no longer have another protocol, but instead we now have to be careful about ensuring we provide the right overloads for both the schedule and cancel functions and need a runtime crash if the contract has been violated.

IMO, this was a place we could have had the type system help us and any external authors of event loops.

I appreciate the argument that it would result in the exposing of lower level functionality to end users, but they don't have to call that, and, if they did it would be safe still.

Possibly a moot point now, but would you have considered a layered approach like that initially proposed if we made use of @_spi to expose it only to event loop implementors?

The targeted relevant changes for this feedback are in 89c57ec.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

Similar comment applies to a few other places too.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

The naming reads (to me) like this function is called at the time when the callback is scheduled (when eventLoop.scheduledCallback is called), as opposed to the time when the callback is scheduled to run.

handleScheduledCallback?

I've updated the name to handleScheduledCallback in 2ec5543.

Question: do we need to be more considerate about name clashes since we're expecting folks to conform their types to this protocol; e.g. should this be something like handleNIOScheduledCallback?

This case isn't quite covered by https://github.com/apple/swift-nio/blob/main/docs/public-api.md, but it seems similar in nature.

WDYT?

@glbrntt did you have any thoughts regarding the namespacing or is it fine the way it is?

The function takes a NIO type, so at worst it could cause an overload? Maybe it's OK the way it is now.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

Now that we're converging on the naming I've made these proper DocC references instead in 5d6ac17.

I've updated the name to handleScheduledCallback in 2ec5543.

Question: do we need to be more considerate about name clashes since we're expecting folks to conform their types to this protocol; e.g. should this be something like handleNIOScheduledCallback?

This case isn't quite covered by https://github.com/apple/swift-nio/blob/main/docs/public-api.md, but it seems similar in nature.

WDYT?

Should there be an optional method to implement when the scheduled callback is cancelled?

Not sure really. I think we'd need to see how people use this a bit more to form an opinion on whether it's useful.

Also, if we were to provide it, would it be cleaner to add a non-optional requirement with a default empty implementation?

non-optional requirement default implementation is Swift's equivalent of optional requirement.

FWIW I think having an onCancel callback is useful. It allows you to centralise the state keeping into a single place.

I don't think we should do this setup inside the benchmark.

Why not? IIUC, that's what startMeasurement is for.

If you're suggesting that I use the setup and teardown closures of the Benchmark() function, then I cannot since I need to actually use these variables in the benchmark. The only alternative would be to make them implicitly unwrapped optionals at global scope, which was pretty gross.

We can reuse ELs across benchmarks so I was thinking it might just be better if we create one EL that we share. I would just define a let eventLoop = MultithreadedEventLoopGroup.singleton.any() at the top.

I'm not sure that's great, I think I'd rather each benchmark do it's own setup and teardown to ensure its unpolluted by the side effects of running other benchmarks.

Where we can, I'm happy to try and use the setup/teardown closure style, if you prefer it, and where we cannot, use this.

It also has the benefit for local reasoning of what's being benchmarked. It's all contained in the call to Benchmark { ... }.

I disagree and I just looked at the code in the benchmark again and we are already defining an EL for exactly this purpose. There is a static EL defined in this file called eventLoop.

The reason why I think it's not important to keep is that it should be irrelevant for the benchmark here. The important part of the benchmark is the scheduling and not how the EL is constructed and shut down so it keeps the benchmark more concise.

IMO we should definitely only have one style here and not mix static EL with one EL per benchmark.

NIT: I personally find callback an overloaded term here since it is mostly used for meaning closure in Swift/ObjC. I liked the previous timer naming.

In this context, isn't it actually a callback being scheduled though?

I liked the previous timer naming.

😭 😰

IMO @glbrntt is right here. This is a callback in the truest sense of what is being scheduled.

Maybe. It was just a nit and I am fine with both

I used to prefer the timer terminology but was convinced by @glbrntt's arguments and it did make the API names much more self-documenting.

non-optional requirement default implementation is Swift's equivalent of optional requirement.

FWIW I think having an onCancel callback is useful. It allows you to centralise the state keeping into a single place.

In this context, isn't it actually a callback being scheduled though?