Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make cancelOnGracefulShutdown operations nonescaping #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tannerdsilva
Copy link

Hello.

As of release v2.4.0, please consider the following function from the public API.

public func cancelOnGracefulShutdown<T: Sendable>(_ operation: @Sendable @escaping () async throws -> T) async rethrows -> T?

I was working on a new project today and while architecting the shutdown procedures, I found myself wanting to use this function a lot. Very useful!

Unfortunately, I'm the kind of programmer that gets hopelessly lost in the details and semantics, and this is one of those scenarios where a perfectly valid implementation sits with me as "sub-optimal". Why? Well, I don't really see why this needs to take an escaping operation. In my personal utopian world, this function would work exactly as it is implemented today, simply with a non-escaping operation arg.

As we know, the primary factor that distinguishes an escaping block and non-escaping block is when the function will be executed. In order to be considered non-escaping, the operation must execute within the lifecycle of the function. To my best interpretation, cancelOnGracefulShutdown looks to abide by this requirement, despite the compiler being unable to "see" this in the content. Indeed, it appears like this operation arg always executes within the boundaries of the root function.

As such, I have implemented this proposal to force the nonescaping semantics on this function. I'm no expert of compilers by any stretch of the imagination, but I suspect the compiler will be able to work much more efficiently with memory with the correct knowledge that the operation never escapes.

}
}
public func cancelOnGracefulShutdown<T:Sendable>(_ operation: @Sendable () async throws -> T) async rethrows -> T? {
return try await withoutActuallyEscaping(operation, do: { operationEscaping in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently not safe to do see: https://forums.swift.org/t/pitch-non-escapable-types-and-lifetime-dependency/69865/23

Once TaskGroup becomes ~Escapable we can make this change here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info. Hadn't seen this - will definitely follow this development and discussion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FranzBusch how sure are we that we cannot deem this "safe enough"?
the TaskGroup escaping would be problematic anyway, and in "normal use" (and the function is short enough to scan that it is) a non-escaping closure should be fine, right?

IIUC John McCall confirmed here that usage like this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just coming back to this. I am very hesitant to make that change even though it might be fine in theory. Do we have a real motivating example where the current API is limiting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a case: a "never-ending" SSE stream in a hummingbird response writer closure thing.
if I remember correctly, the stream writer is an inout parameter, so I could not wrap the producing loop it in a cancelOnGracefulShutdown block.

in my case I was able to rework the logic to be AsyncSequence-based and use for await ... in myStream.cancelOnGracefulShutdown() - maybe even cleaner, but it still felt like a real limitation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That's really unfortunate. Let's keep an eye on this and see if more such problems happen and then we can re-evaluate if we should continue with this here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants