Skip to content

Conversation

@sidepelican
Copy link
Contributor

Issue: #179

cancelOnGracefulShutdown has no code paths that return a nil value.

///
/// - Parameter operation: The actual operation.
public func cancelOnGracefulShutdown<T: Sendable>(_ operation: @Sendable @escaping () async throws -> T) async rethrows -> T? {
public func cancelOnGracefulShutdown<T: Sendable>(_ operation: @Sendable @escaping () async throws -> T) async rethrows -> T {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we gotta be a bit careful here since this is a breaking change. Can we deprecate this method and introduce the same one. This one can call the new one to avoid duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also concerned about that, but couldn't find a suitable solution.

We could provide an overload method without an ambiguous error as follows:

@available(*, deprecated)
@_disfavoredOverload
public func cancelOnGracefulShutdown<T: Sendable>(_ operation: @Sendable @escaping () async throws -> T) async rethrows -> T? {
    ...
}

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

However, this overload does not address the source-breaking issue.
Since T can be assigned to T?, the new method will be used automatically.

// OK
let value: Int? = await cancelOnGracefulShutdown {
    return 42
}

Nevertheless, if there were a use case like the following, it would break existing code, and we couldn't resolve it with an overload:

// Source breaking!
let value = await cancelOnGracefulShutdown {
    return 42
}
if let value {
    ...
}

Aside from providing an overload, the only other option would be to use a different name, but I didn’t think it was appropriate to rename this function.

I just can't think of a good approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's completely true. We gotta introduce a new name here sadly. Let's deprecate the current method and introduce a new one called cancelWhenGracefulShutdown. It isn't great but it gets us moving forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have implemented cancelWhenGracefulShutdown.

@sidepelican sidepelican requested a review from FranzBusch April 26, 2024 12:19
Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Thanks!

@FranzBusch
Copy link
Contributor

@swift-server-bot test this please

@FranzBusch FranzBusch merged commit 877d749 into swift-server:main Apr 26, 2024
@simonjbeaumont simonjbeaumont added the 🆕 semver/minor Adds new public API. label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants