Add manual control to NIOLockedValueBox #2786

glbrntt · 2024-07-12T08:17:40Z

Motivation:

NIOLockedValueBox has a 'safer' API than NIOLock as it only provides scoped access to its boxed value. NIOLock requires users to only access protected state while the lock is acquired. As such NIOLockedValueBox should be preferred where possible. However, there are cases where manual control must be used (such as storing a continuation) and users must use a NIOLock for this.

There are two downsides to this:

All other access to the protected state must use the NIOLock API putting the onus on the developer to only access the protected state while the lock is held.
NIOLock can't store its protected state inline which typically results in users storing it on a class.

Modifications:

Add an 'unsafe' view to NIOLockedValueBox which allows users to manually control the lock and access its protected state
Update NIOAsyncWriter and NIOThrowingAsyncSequenceProducer to use NIOLockedValueBox

Result:

Safer locking API is used in more places
Fewer allocations

Lukasa · 2024-07-15T10:31:52Z

Sources/NIOConcurrencyHelpers/NIOLockedValueBox.swift

+
+        /// Mutate the value, assuming the lock has been acquired manually.
+        @inlinable
+        public func withValueAssumingLockIsAcquired<T>(


The spelling for this on OSAllocatedUnfairLock is withLockUnchecked. It might be worth us aligning with that.

While we're here, let's avoid using single-letter generic type parameter names. ReturnValue is probably better.

Do you think Unchecked is also a more appropriate name for the type as well? Wasn't so sure on the naming...

I think Unchecked is better. We normally use Unsafe when it causes undefined behaviour which manual locking shouldn't

IIUC withLockUnchecked has different semantics: it still acquires the lock but does not check the sendability of the closure passed in or its return value.

This is different: it assumes that the caller has already called lock() and that they will subsequently call unlock().

re: Unsafe, I think depending on what is stored in the locks state, this could be unsafe if you didn't acquire the lock first.

Ok, let's leave the name and fix the generic type parameters.

And FWIW, getting manual locking wrong can absolutely cause UB.

Lukasa · 2024-07-15T10:43:53Z

Sources/NIOCore/AsyncSequences/NIOThrowingAsyncSequenceProducer.swift

+            @usableFromInline
+            var delegate: Delegate?
+            @usableFromInline
+            var didSuspend: (@Sendable () -> Void)?


This value was never mutated, so I think we can make it let and remove it from the lock.

It's mutated from _setDidSuspend on L417 (it's only used for testing though)

Motivation: NIOLockedValueBox has a 'safer' API than NIOLock as it only provides scoped access to its boxed value. NIOLock requires users to only access protected state while the lock is acquired. As such NIOLockedValueBox should be preferred where possible. However, there are cases where manual control must be used (such as storing a continuation) and users must use a NIOLock for this. There are two downsides to this: 1. All other access to the protected state must use the NIOLock API putting the onus on the developer to only access the protected state while the lock is held. 2. NIOLock can't store its protected state inline which typically results in users storing it on a class. Modifications: - Add an 'unsafe' view to NIOLockedValueBox which allows users to manually control the lock and access its protected state - Update NIOAsyncWriter and NIOThrowingAsyncSequenceProducer to use NIOLockedValueBox Result: - Safer locking API is used in more places - Fewer allocations

FranzBusch

LGTM!

* apple/main: (26 commits) [GHA] Only format Swift files that are in Git index (apple#2797) Ignore format commit from git blame (apple#2796) Adopt swift-format (apple#2794) Disable warnings as errors on Swift 6 and main (apple#2793) ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) (apple#2791) Add manual control to NIOLockedValueBox (apple#2786) [GHA] Cxx interoperability compatibility and integration tests check (apple#2790) [GHA] Introduce reusable matrix workflow (apple#2789) Fix benchmark thresholds update script (apple#2783) [GHA] Broken symlink and format check (apple#2787) [GHA] Add license header check (apple#2781) Improved documentation for HTTP Parts to clarify how often each part is received (apple#2775) [GHA] Download the scripts to make workflow reusable (apple#2785) Combine the two NIOAsyncChannel channel handlers (apple#2779) [GHA] Benchmark job (apple#2780) [GHA] Move docs check to script (apple#2776) Add benchmark for creating NIOAsyncChannel (apple#2774) Avoid creating a yield ID counter per async writer (apple#2768) [GHA] Unacceptable language check (apple#2766) Allow in-place mutation of `NIOLoopBoundBox.value` (apple#2771) ... # Conflicts: # Sources/NIOPosix/BSDSocketAPICommon.swift # Sources/NIOPosix/GetaddrinfoResolver.swift # Sources/NIOPosix/HappyEyeballs.swift

glbrntt requested a review from FranzBusch July 12, 2024 08:17

glbrntt added the semver/minor Adds new public API. label Jul 12, 2024

Lukasa reviewed Jul 15, 2024

View reviewed changes

glbrntt requested a review from Lukasa July 16, 2024 10:29

glbrntt marked this pull request as ready for review July 16, 2024 11:09

glbrntt added 2 commits July 18, 2024 08:40

Better generic name

c41e850

glbrntt force-pushed the avoid-storage-allocs branch from c3af639 to c41e850 Compare July 18, 2024 07:41

glbrntt enabled auto-merge (squash) July 18, 2024 07:42

FranzBusch approved these changes Jul 18, 2024

View reviewed changes

glbrntt merged commit 223b0e8 into apple:main Jul 18, 2024
30 of 35 checks passed

glbrntt deleted the avoid-storage-allocs branch July 18, 2024 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add manual control to NIOLockedValueBox #2786

Add manual control to NIOLockedValueBox #2786

glbrntt commented Jul 12, 2024

Lukasa Jul 15, 2024

glbrntt Jul 15, 2024

FranzBusch Jul 15, 2024

glbrntt Jul 15, 2024 •

edited

Loading

glbrntt Jul 15, 2024

Lukasa Jul 17, 2024

Lukasa Jul 15, 2024

glbrntt Jul 15, 2024

FranzBusch left a comment

Add manual control to NIOLockedValueBox #2786

Add manual control to NIOLockedValueBox #2786

Conversation

glbrntt commented Jul 12, 2024

Lukasa Jul 15, 2024

Choose a reason for hiding this comment

glbrntt Jul 15, 2024

Choose a reason for hiding this comment

FranzBusch Jul 15, 2024

Choose a reason for hiding this comment

glbrntt Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

glbrntt Jul 15, 2024

Choose a reason for hiding this comment

Lukasa Jul 17, 2024

Choose a reason for hiding this comment

Lukasa Jul 15, 2024

Choose a reason for hiding this comment

glbrntt Jul 15, 2024

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

glbrntt Jul 15, 2024 •

edited

Loading