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.

Sign up for GitHub

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
Open

Make cancelOnGracefulShutdown operations nonescaping #174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 38 additions & 36 deletions Sources/ServiceLifecycle/GracefulShutdown.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,42 +74,44 @@ enum ValueOrGracefulShutdown<T: Sendable>: Sendable {
/// Cancels the closure when a graceful shutdown was triggered.
///
/// - Parameter operation: The actual operation.
public func cancelOnGracefulShutdown<T: Sendable>(_ operation: @Sendable @escaping () async throws -> T) async rethrows -> T? {
return try await withThrowingTaskGroup(of: ValueOrGracefulShutdown<T>.self) { group in
group.addTask {
let value = try await operation()
return .value(value)
}

group.addTask {
for try await _ in AsyncGracefulShutdownSequence() {
return .gracefulShutdown
}

throw CancellationError()
}

let result = try await group.next()
group.cancelAll()

switch result {
case .value(let t):
return t
case .gracefulShutdown:
switch try await group.next() {
case .value(let t):
return t
case .gracefulShutdown:
fatalError("Unexpectedly got gracefulShutdown from group.next()")

case nil:
fatalError("Unexpectedly got nil from group.next()")
}

case nil:
fatalError("Unexpectedly got nil from group.next()")
}
}
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.

return try await withThrowingTaskGroup(of: ValueOrGracefulShutdown<T>.self) { group in
group.addTask {
let value = try await operationEscaping()
return .value(value)
}

group.addTask {
for try await _ in AsyncGracefulShutdownSequence() {
return .gracefulShutdown
}

throw CancellationError()
}

let result = try await group.next()
group.cancelAll()

switch result {
case .value(let t):
return t
case .gracefulShutdown:
switch try await group.next() {
case .value(let t):
return t
case .gracefulShutdown:
fatalError("Unexpectedly got gracefulShutdown from group.next()")

case nil:
fatalError("Unexpectedly got nil from group.next()")
}

case nil:
fatalError("Unexpectedly got nil from group.next()")
}
}
})
}

extension Task where Success == Never, Failure == Never {
Expand Down