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 NIOLoopBound(Box) uncheckedValue #2515

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

Conversation

natikgadzhi
Copy link
Contributor

Motivation

Modifications

  • Added init and public var uncheckedValue properties to both NIOLoopBound and NIOLoopBoundBox.

Review

  • Tests pass locally.
  • How should we test that NIOLoopBound.init(uncheckedEventLoop:) calls assertInEventLoop? EmbeddedEventLoop seems to be final — how do I mock the assert function on it elegantly, without copy and pasting all the protocol conformance stuff?

@weissi, sorry, I found a few really nice rabbit holes in swift-syntax and got distracted 👀

**Summary**:
This commit adds unchecked version of init and value get+set of
`NIOLoopBound` and `NIOLoopBoundBox`.

**Motivation**:
- Closes apple#2506.

**Changes**:
- Added `init` and `public var uncheckedValue` properties to both
  `NIOLoopBound` and `NIOLoopBoundBox`.

**Review & help**
- [x] Tests pass locally.
- [ ] How should we test that `NIOLoopBound.init(uncheckedEventLoop:)`
  calls `assertInEventLoop`? `EmbeddedEventLoop` seems to be final — how
  do I mock the assert function on in elegantly, without copy and
    pasting all the protocol conformance stuff?
@@ -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
/// 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
Contributor

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
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.

@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

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.

@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

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.

Comment on lines +69 to +79
@inlinable
public var uncheckedValue: Value {
get {
self._eventLoop.assertInEventLoop()
return self._value
}
set {
self._eventLoop.assertInEventLoop()
self._value = newValue
}
}
Copy link
Contributor

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
Contributor

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

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

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.

@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 Sep 11, 2023
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.

NIOLoopBound should get unchecked variants that are truly free in release mode
5 participants