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

Add NIOLoopBound(Box) uncheckedValue #2515

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

Add NIOLoopBound(Box) uncheckedValue #2515

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
62 changes: 57 additions & 5 deletions Sources/NIOCore/NIOLoopBound.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,18 @@ public struct NIOLoopBound<Value>: @unchecked Sendable {
@usableFromInline
/* private */ var _value: Value

/// Initialise a ``NIOLoopBound`` to `value` with the precondition that the code is running on `eventLoop`.
/// Initialize a ``NIOLoopBound`` to `value` with the precondition that the code is running on `eventLoop`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tomato tomato, but thought there are more initializes in our codebase than initialises.

@inlinable
public init(_ value: Value, eventLoop: EventLoop) {
eventLoop.preconditionInEventLoop()
self.init(value, uncheckedEventLoop: eventLoop)
}

@inlinable
/// Initialize a ``NIOLoopBound`` to `value` with _an assertion_ that the code is running on `uncheckedEventLoop`.
/// Unlike a precondition check, ``EventLoop/assertInEventLoop(file:line:)`` only performs the check in debug configuration, so the check is free in release configuration.
public init(_ value: Value, uncheckedEventLoop eventLoop: EventLoop) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few hours after I pushed this up, I realized that having an unchecked init does not necessarily win the user that much time — you init once. Perhaps if you have to init a lot of objects that makes sense, but uncheckedValue seems more useful, in comparison.

Copy link
Member

Choose a reason for hiding this comment

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

uncheckedValue is basically UnsafeTransfer which just puts the trust into the users hand that they now where things are going and that it is safe to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is worth keeping: while you only init once, you typically shouldn't store one of these: instead, you should use it to effect a transfer.

Copy link
Member

Choose a reason for hiding this comment

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

I think unchecked isn't quite the right name for this init but I can't come up with a better one right now. It is not completely unchecked but rather just asserts than preconditions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unchecked is fine for that: it's what Range uses, for example.

eventLoop.assertInEventLoop()
self._eventLoop = eventLoop
self._value = value
}
Expand All @@ -52,6 +60,23 @@ public struct NIOLoopBound<Value>: @unchecked Sendable {
self._value = newValue
}
}

/// Access the `value` with the assertion that the code is running on `eventLoop`.
///
/// Unlike ``NIOLoopBound/value``, this performs the assertion in debug configuration only, so it's
/// cheaper, and still performs the precondition check in debug mode.
/// - note: ``NIOLoopBound`` itself is value-typed, so any writes will only affect the current value.
@inlinable
public var uncheckedValue: Value {
get {
self._eventLoop.assertInEventLoop()
return self._value
}
set {
self._eventLoop.assertInEventLoop()
self._value = newValue
}
}
Comment on lines +69 to +79
Copy link
Member

Choose a reason for hiding this comment

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

What if we just provide a second type that does asserts or preconditions. Alternatively what about just making the whole precondition/assert behaviour configurable on init?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this pattern is better: I don't see a good reason to make this a type or a runtime distinction. It's good to be able to see this in the code, as it's fundamentally a safety-breaking optimization.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, if we say unchecked is fine as a spelling then I am happy with this init and property as well.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I'm also thinking a type would be better. It's how the rest of Swifts unsafe/unchecked APIs usually work e.g. UncheckedContinuation, Unsafe*Pointer, @unchecked Sendable.

It may also enable us to drop the stored EventLoop property in release modes, once DEBUG is properly propagated in SwiftPM (haven't checked recently), making this actually a zero cost abstraction.

Copy link
Member

Choose a reason for hiding this comment

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

One type is better here: I often want mixed use: First time in a function: checked, follow-ups: unchecked. Different type would be pain and would likely push me to use unchecked everywhere.

Copy link
Member

@dnadoba dnadoba Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the number of safe types with unsafe functions vastly dwarfs the number of unsafe types

All the withUnsafe* functions I can think of vend you types that have Unsafe or Unchecked in the name. About which functions do you think?
I'm only aware of unsafeDowncast, unsafeBitCast and [Closed]Range(uncheckedBounds:) which result in a type that doesn't have Unsafe in the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

[Closed]Range is the controlling example. Range is not an unsafe type, but you can construct it in an unsafe way. You should not audit your entire codebase because you want to allow people to opt out of enforcement of an invariant. Imagine for a moment the counterfactual, where we had UnsafeRange. Such a type is borderline useless, because to support people using it you have to litter it all over your codebase. This makes the "grep for Unsafe" strategy very weak, because almost all of the places holding an UnsafeRange are not doing things that are unsafe.

Similarly here: you want to draw attention to the places where the invariants are being broken, not to where they are being observed.

Copy link
Member

@dnadoba dnadoba Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree that you need to look at where the invariants are broken but you need to look at the creation for that for NIOLoopBound. [Closed]Range(uncheckedBounds:) only has one unsafe/unchecked API and everything else is safe/checked. You only need to audit the use of this initialiser, nothing else. If [Closed]Range(uncheckedBounds:) is safe or not can be determined by just its parameters passed into the initialiser and doesn't have any other dependencies.

You can't say in isolation if loopBound.uncheckedValue is safe or not. You need to go and look at the creation of the NIOLoopBound and trace back on which EventLoop it was created. We are again more like Unsafe*Pointer. The creation and operations of Unsafe*Pointer are more complex and dependent on where it is coming from.

We are proposing a set of duplicated APIs which make all operations on NIOLoopBound unsafe/unchecked.
This is more in line with the UnsafePointer APIs where almost all operation are unsafe.

This design also gives the wrong impression that NIOLoopBound.value is always safe and only uncheckedValue is unsafe. With this PR value becomes unsafe as well as you might have initialised it with NIOLoopBound(_:uncheckedEventLoop).

Copy link
Member

Choose a reason for hiding this comment

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

Concretely, I'm proposing this API:

public struct NIOUncheckedLoopBound<Value> {
    public init(_ value: Value, eventLoop: EventLoop) {}
    public init(_ value: NIOLoopBound<Value>) {}
    public init(_ value: NIOLoopBoundBox<Value>) {}
    
    public var value: Value {
        get {}
    }
}

public final class NIOUncheckedLoopBoundBox<Value> {
    public init(_ value: Value, eventLoop: EventLoop) {}
    public init(_ value: NIOLoopBound<Value>) {}
    public init(_ value: NIOLoopBoundBox<Value>) {}
    
    public var value: Value {
        get {}
        set {}
    }
}

If NIOLoopBound.value is to expensive you can still wrap it in NIOUncheckedLoopBound(myLoopBoundValue).value. Its more verbose but that's a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoa, thank you, @dnadoba, @Lukasa, and @weissi for the discussion! I'm learning quite a lot from you here.

I'm not attached to the current implementation — as I learn more and if we decide to change it, I'm happy to rewrite it.

You can't say in isolation if loopBound.uncheckedValue is safe or not. You need to go and look at the creation of the NIOLoopBound and trace back on which EventLoop it was created.

I agree, and I agree it's different from the Range example.

We are proposing a set of duplicated APIs which make all operations on NIOLoopBound unsafe/unchecked.
This is more in line with the UnsafePointer APIs where almost all operation are unsafe.

This design also gives the wrong impression that NIOLoopBound.value is always safe and only uncheckedValue is unsafe. With this PR value becomes unsafe as well as you might have initialised it with NIOLoopBound(_:uncheckedEventLoop).

If we choose to stick with the current implementation, I think I should at the very least comment and document on that behavior.

Such a type is borderline useless, because to support people using it you have to litter it all over your codebase.

@Lukasa, with the Range example, I'd fully agree. For NIOLoopBound, though — we're not passing LoopBound around in the NIO codebase right now, right? So in theory, sure, we could do two separate structures. The weight of supporting two different structures would then be on the user. Perhaps we could provide a NIOLoopBound as a protocol, and make a safe and unchecked implementations of that protocol, so that users could accept some NIOLoopBound and deal with them that way?

Again, I feel that I'm out of my depth in the API design discussion, but I'm delighted to be part of it and listen in. Feel free to "well actually" me on any points I'm bringing up.

}

/// ``NIOLoopBoundBox`` is an always-`Sendable`, reference-typed container allowing you access to ``value`` if and
Expand All @@ -77,19 +102,29 @@ public final class NIOLoopBoundBox<Value>: @unchecked Sendable {
@usableFromInline
/* private */var _value: Value

/// Initialize a ``NIOLoopBoundBox`` to `value` with the an assertion that the code is running on `eventLoop`.
///
/// - note: Unlike ``NIOLoopBoundBox/init(_:eventLoop:)``, this performs ``EventLoop/assertInEventLoop(file:line:)`` instead of a precondition check, which is free in release mode.
@inlinable
public init(_ value: Value, uncheckedEventLoop eventLoop: EventLoop) {
eventLoop.assertInEventLoop()
self._eventLoop = eventLoop
self._value = value
}

@inlinable
internal init(_value value: Value, uncheckedEventLoop eventLoop: EventLoop) {
internal init(_ value: Value, notVerifyingEventLoop eventLoop: EventLoop) {
self._eventLoop = eventLoop
self._value = value
}

/// Initialise a ``NIOLoopBoundBox`` to `value` with the precondition that the code is running on `eventLoop`.
/// Initialize a ``NIOLoopBoundBox`` to `value` with the precondition that the code is running on `eventLoop`.
@inlinable
public convenience init(_ value: Value, eventLoop: EventLoop) {
// This precondition is absolutely required. If not, it were possible to take a non-Sendable `Value` from
// _off_ the ``EventLoop`` and transport it _to_ the ``EventLoop``. That would be illegal.
eventLoop.preconditionInEventLoop()
self.init(_value: value, uncheckedEventLoop: eventLoop)
self.init(value, uncheckedEventLoop: eventLoop)
}

/// Initialise a ``NIOLoopBoundBox`` that is empty (contains `nil`), this does _not_ require you to be running on `eventLoop`.
Expand All @@ -104,7 +139,7 @@ public final class NIOLoopBoundBox<Value>: @unchecked Sendable {
// - Because of Swift's Definitive Initialisation (DI), we know that we did write `self._value` before `init`
// returns.
// - The only way to ever write (or read indeed) `self._value` is by proving to be inside the `EventLoop`.
return .init(_value: nil, uncheckedEventLoop: eventLoop)
return .init(nil, notVerifyingEventLoop: eventLoop)
}

/// Access the `value` with the precondition that the code is running on `eventLoop`.
Expand All @@ -121,5 +156,22 @@ public final class NIOLoopBoundBox<Value>: @unchecked Sendable {
self._value = newValue
}
}

/// Access the `value` with the assertion that the code is running on `eventLoop`.
///
/// - note: ``NIOLoopBoundBox`` itself is reference-typed, so any writes will affect anybody sharing this reference.
@inlinable
public var uncheckedValue: Value {
get {
self._eventLoop.assertInEventLoop()
return self._value
}
set {
self._eventLoop.assertInEventLoop()
self._value = newValue
}
}


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: Run swiftformat, come on.

}