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

Add async version of NIOThreadPool.runIfActive #2566

Merged
merged 6 commits into from Oct 25, 2023

Conversation

adam-fowler
Copy link
Contributor

@adam-fowler adam-fowler commented Oct 23, 2023

Add async version of NIOThreadPool.runIfActive

Motivation:

Currently when using NIOThreadPool in a Swift concurrency context you still need an EventLoop and the call will make two hops, one to the EventLoop and then one back to your Task.

Modifications:

Added NIOThreadPool.runIfActive(_) async throws

Result:

You don't need an EventLoop to use NIOThreadPool

This is the first stage of completing #2565

@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Oct 23, 2023
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks, a couple of quick notes here.

@@ -292,6 +292,48 @@ extension NIOThreadPool {
}
}

@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
extension NIOThreadPool {
#if swift(>=5.7)
Copy link
Contributor

Choose a reason for hiding this comment

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

We no longer support older versions of Swift than 5.7, so we can drop this conditional compilation step.

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've removed the older version

Tests/NIOPosixTests/NIOThreadPoolTest.swift Show resolved Hide resolved
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Generally looking good, a few quick notes on the API.

try await self._runIfActive(body)
}

private func _runIfActive<T>(_ body: @escaping () throws -> T) async throws -> T {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this inner function now, and so we can avoid dropping the @Sendable annotation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// - parameters:
/// - body: The closure which performs some blocking work to be done on the thread pool.
/// - returns: result of the passed closure.
@preconcurrency
Copy link
Contributor

Choose a reason for hiding this comment

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

We can drop preconcurrency, this method doesn't predate concurrency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// - body: The closure which performs some blocking work to be done on the thread pool.
/// - returns: result of the passed closure.
@preconcurrency
public func runIfActive<T>(_ body: @escaping @Sendable () throws -> T) async throws -> T {
Copy link
Contributor

Choose a reason for hiding this comment

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

T: Sendable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, thanks!

@Lukasa Lukasa enabled auto-merge (squash) October 25, 2023 15:24
@Lukasa Lukasa merged commit 95a4eaa into apple:main Oct 25, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants