-
Notifications
You must be signed in to change notification settings - Fork 644
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
Introduce assumeIsolated()
methods on EventLoop
, EventLoopPromise
and EventLoopFuture
#2657
Conversation
assumeIsolated()
methods on EventLoop
, `EventLoopPromis…assumeIsolated()
methods on EventLoop
, EventLoopPromise
and EventLoopFuture
c1dd094
to
10d265b
Compare
e66cdbb
to
a9582cf
Compare
…e` and `EventLoopFuture` > All methods/types are currently `internal` so we don't have to bikeshed just yet but we can move forward to get `NIOCore` warning free under strict concurrency # Motivation Methods on the above types are often called from the same event loop; however, we cannot prove to the compiler that this is true so we had to mark many methods on those types with `@Sendable` or require the generic type to be `Sendable`. This leads to unnecessary usage of `NIOLoopBound` when instead we should just dynamically assert that we are on the event loop. @dnadoba opened a very similar PR apple#2228. # Modification This PR provides a method called `assumeIsolated()` on the three types that returns a type which re-declaration of all methods of the wrapped typed that have `Sendable` annotations. This new type is asserting at runtime that we are on the right event loop; hence, we don't need a `Sendable` value. # Result This PR makes it easier for our adopters to avoid newly introduced `Sendable` warnings
a9582cf
to
1815a22
Compare
@swift-server-bot test this please |
/// A struct wrapping an ``EventLoopFuture`` that ensures all calls to any method on this struct | ||
/// are coming from the event loop of the future. | ||
@usableFromInline | ||
struct AssumeIsolated: @unchecked Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised that none of the methods on here return the caller back an AssumeIsolated
future? This would make chaining much easier. Also, similar to above, it feels strange that we can move the AssumeIsolated
future across isolation domains because it's Sendable
. I think we want to make the assumption when we create it and have it not be Sendable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally did not return the AssumeIsolated
from the methods but rather the regular types since those types contain way more methods and I don't want to duplicate everything. assumedIsolated()
should really be a no-op in the compiler since it is nothing more than an asserting wrapper and be optimised away in release builds. We could duplicate all the methods or make the underlying type public but I leaned towards having users to add more assumeIsolated()
calls rather.
719d99f
to
c622050
Compare
c622050
to
f7c4d8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits but otherwise I think this looks pretty good
Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift
Outdated
Show resolved
Hide resolved
/// - parameters: | ||
/// - value: The successful result of the operation. | ||
@inlinable | ||
func succeed(_ value: Value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No fail
counterpart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need fail
counter part since that doesn't require Sendable
values. I really wanted to avoid replicating all methods on the new types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By that logic we don't need complete
either (I don't think we should remove it though...)
78be5dd
to
4a731ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM module the fail
on the promise. Not a hill I'll die on though.
Motivation
Methods on the above types are often called from the same event loop; however, we cannot prove to the compiler that this is true so we had to mark many methods on those types with
@Sendable
or require the generic type to beSendable
. This leads to unnecessary usage ofNIOLoopBound
when instead we should just dynamically assert that we are on the event loop. @dnadoba opened a very similar PR #2228.Modification
This PR provides a method called
assumeIsolated()
on the three types that returns a type which re-declaration of all methods of the wrapped typed that haveSendable
annotations. This new type is asserting at runtime that we are on the right event loop; hence, we don't need aSendable
value.Result
This PR makes it easier for our adopters to avoid newly introduced
Sendable
warnings