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

Adopt Mutex where available #70

Merged
merged 4 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 14
"mallocCountTotal" : 13
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 14
"mallocCountTotal" : 13
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 14
"mallocCountTotal" : 13
}
57 changes: 50 additions & 7 deletions Sources/HTTPTypes/HTTPFields.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
//
//===----------------------------------------------------------------------===//

#if compiler(>=6.0)
import Synchronization
#endif // compiler(>=6.0)

/// A collection of HTTP fields. It is used in `HTTPRequest` and `HTTPResponse`, and can also be
/// used as HTTP trailer fields.
///
Expand All @@ -21,13 +25,19 @@
/// `HTTPFields` adheres to modern HTTP semantics. In particular, the "Cookie" request header field
/// is split into separate header fields by default.
public struct HTTPFields: Sendable, Hashable {
private final class _Storage: @unchecked Sendable, Hashable {
private class _Storage: @unchecked Sendable, Hashable {
var fields: [(field: HTTPField, next: UInt16)] = []
var index: [String: (first: UInt16, last: UInt16)]? = [:]
let lock = LockStorage.create(value: ())

required init() {
}

func withLock<Result>(_ body: () throws -> Result) rethrows -> Result {
fatalError()
}

var ensureIndex: [String: (first: UInt16, last: UInt16)] {
self.lock.withLockedValue { _ in
self.withLock {
if let index = self.index {
return index
}
Expand All @@ -45,10 +55,10 @@ public struct HTTPFields: Sendable, Hashable {
}
}

func copy() -> _Storage {
let newStorage = _Storage()
func copy() -> Self {
let newStorage = Self()
newStorage.fields = self.fields
self.lock.withLockedValue { _ in
self.withLock {
newStorage.index = self.index
}
return newStorage
Expand Down Expand Up @@ -99,7 +109,40 @@ public struct HTTPFields: Sendable, Hashable {
}
}

private var _storage = _Storage()
#if compiler(>=6.0)
@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to fix this on Swift 5.9/5.10.

/__w/swift-http-types/swift-http-types/Sources/HTTPTypes/HTTPFields.swift:113:63: error: unrecognized platform name 'visionOS'
    @available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by switching to a compiler version check instead of a canImport check.

private final class _StorageWithMutex: _Storage, @unchecked Sendable {
let mutex = Mutex<Void>(())

override func withLock<Result>(_ body: () throws -> Result) rethrows -> Result {
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually compile without warnings under strict concurrency? The closure that Mutex takes is sending whereas ours is not. This should produce a warning/error.

Copy link

Choose a reason for hiding this comment

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

Needing dynamic dispatch is also a little gnarly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried .enableExperimentalFeature("StrictConcurrency") and didn't get any errors, but added sending in 948fe6c to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think dynamic dispatch is still better than an extra allocation for the lock?

Copy link
Contributor Author

@guoye-zhang guoye-zhang Oct 29, 2024

Choose a reason for hiding this comment

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

Had to revert 948fe6c due to it breaking Swift 5.9/5.10

try self.mutex.withLock { _ in
try body()
}
}
}
#endif // compiler(>=6.0)

private final class _StorageWithNIOLock: _Storage, @unchecked Sendable {
let lock = LockStorage.create(value: ())

override func withLock<Result>(_ body: () throws -> Result) rethrows -> Result {
try self.lock.withLockedValue { _ in
try body()
}
}
}

private var _storage = {
#if compiler(>=6.0)
if #available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *) {
_StorageWithMutex()
} else {
_StorageWithNIOLock()
}
#else // compiler(>=6.0)
_StorageWithNIOLock()
#endif // compiler(>=6.0)
}()

/// Create an empty list of HTTP fields
public init() {}
Expand Down