Skip to content

Make ZippyJSONDecoder thread-safe and conform to Sendable #70

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

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

Make ZippyJSONDecoder thread-safe and conform to Sendable #70

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
135 changes: 108 additions & 27 deletions Sources/ZippyJSON/ZippyJSONDecoder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//Copyright (c) 2018 Michael Eisel. All rights reserved.

import os
import Foundation
import ZippyJSONCFamily
import JJLISO8601DateFormatter
Expand Down Expand Up @@ -37,19 +38,15 @@ func isOnSimulator() -> Bool {
#endif
}

public final class ZippyJSONDecoder {
public final class ZippyJSONDecoder: Sendable {
Copy link
Author

@JaviSoto JaviSoto Feb 4, 2025

Choose a reason for hiding this comment

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

This compiles without having to resort to @unchecked Sendable, so we get a compile-time guarantee of thread-safety :)

Copy link
Owner

@michaeleisel michaeleisel Feb 4, 2025

Choose a reason for hiding this comment

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

very nice!

@available(*, deprecated, message: "This flag is deprecated because full-precision parsing speed is now on par with imprecise, so it will just always use full-precision")
public var zjd_fullPrecisionFloatParsing = true
public let zjd_fullPrecisionFloatParsing = true

@Synchronized
private static var _zjd_suppressWarnings: Bool = false
public static var zjd_suppressWarnings: Bool {
get {
return _zjd_suppressWarnings
}
set {
objc_sync_enter(self)
defer { objc_sync_exit(self) }
Comment on lines -49 to -50
Copy link
Author

@JaviSoto JaviSoto Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's safe to use this API from Swift, especially when combined with other primitives which callers of this API may be using. Since I had to make other mutable properties thread-safe, I just used the same @Synchronized primitive for all of them

Copy link
Owner

@michaeleisel michaeleisel Feb 4, 2025

Choose a reason for hiding this comment

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

interesting, i'd be curious to see any resources you have indicating it's unsafe, although using a lock does seem simpler

_zjd_suppressWarnings = newValue
}
get { _zjd_suppressWarnings }
set { _zjd_suppressWarnings = newValue }
}

private func createContext(string: UnsafePointer<Int8>, length: Int) -> ContextPointer {
Expand Down Expand Up @@ -167,42 +164,66 @@ public final class ZippyJSONDecoder {
}
}

@Synchronized
private var _userInfo: [CodingUserInfoKey : Any] = [:]
public var userInfo: [CodingUserInfoKey : Any] {
get { _userInfo }
set { _userInfo = newValue }
}

public var userInfo: [CodingUserInfoKey : Any] = [:]

public var nonConformingFloatDecodingStrategy: NonConformingFloatDecodingStrategy
@Synchronized
private var _nonConformingFloatDecodingStrategy: NonConformingFloatDecodingStrategy
public var nonConformingFloatDecodingStrategy: NonConformingFloatDecodingStrategy {
Copy link
Owner

@michaeleisel michaeleisel Feb 5, 2025

Choose a reason for hiding this comment

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

why can't public var have @Synchronized on it and we don't need to wrap? is it to avoid exposing @Synchronized?

get { _nonConformingFloatDecodingStrategy }
set { _nonConformingFloatDecodingStrategy = newValue }
}

public enum NonConformingFloatDecodingStrategy {
public enum NonConformingFloatDecodingStrategy: Sendable {
case `throw`
case convertFromString(positiveInfinity: String, negativeInfinity: String, nan: String)
}

public var dataDecodingStrategy: DataDecodingStrategy
@Synchronized
private var _dataDecodingStrategy: DataDecodingStrategy
public var dataDecodingStrategy: DataDecodingStrategy {
get { _dataDecodingStrategy }
set { _dataDecodingStrategy = newValue }
}

public enum DataDecodingStrategy {
public enum DataDecodingStrategy: Sendable {
case deferredToData
case base64
case custom((Decoder) throws -> Data)
case custom(@Sendable (Decoder) throws -> Data)
Copy link
Owner

@michaeleisel michaeleisel Feb 4, 2025

Choose a reason for hiding this comment

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

this appears to be a breaking change, but maybe it's worth it

}

public enum KeyDecodingStrategy {
public enum KeyDecodingStrategy: Sendable {
case useDefaultKeys
case convertFromSnakeCase
case custom(([CodingKey]) -> CodingKey)
case custom(@Sendable ([CodingKey]) -> CodingKey)
}

public var keyDecodingStrategy: KeyDecodingStrategy
@Synchronized
private var _keyDecodingStrategy: KeyDecodingStrategy
public var keyDecodingStrategy: KeyDecodingStrategy {
get { _keyDecodingStrategy }
set { _keyDecodingStrategy = newValue }
}

public enum DateDecodingStrategy {
public enum DateDecodingStrategy: Sendable {
case deferredToDate
case secondsSince1970
case millisecondsSince1970
case iso8601
case formatted(DateFormatter)
case custom((Decoder) throws -> Date)
case custom(@Sendable (Decoder) throws -> Date)
}

public var dateDecodingStrategy: DateDecodingStrategy
@Synchronized
private var _dateDecodingStrategy: DateDecodingStrategy
public var dateDecodingStrategy: DateDecodingStrategy {
get { _dateDecodingStrategy }
set { _dateDecodingStrategy = newValue }
}

var convertCase: Bool {
get {
Expand All @@ -216,10 +237,10 @@ public final class ZippyJSONDecoder {
}

public init() {
keyDecodingStrategy = .useDefaultKeys
dataDecodingStrategy = .base64
dateDecodingStrategy = .deferredToDate
nonConformingFloatDecodingStrategy = .throw
_keyDecodingStrategy = .useDefaultKeys
_dataDecodingStrategy = .base64
_dateDecodingStrategy = .deferredToDate
_nonConformingFloatDecodingStrategy = .throw
}
}

Expand Down Expand Up @@ -1057,3 +1078,63 @@ extension __JSONDecoder : SingleValueDecodingContainer {

// End
}

@propertyWrapper
private final class Synchronized<Value>: @unchecked Sendable {
private let lock: LockProtocol
Copy link
Owner

@michaeleisel michaeleisel Feb 5, 2025

Choose a reason for hiding this comment

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

so it appears that this design creates like 4 of these per decoder? i believe that should be fast, but i may verify

private var _wrappedValue: Value

init(wrappedValue: Value) {
self.lock = Lock()
_wrappedValue = wrappedValue
}

var wrappedValue: Value {
get {
lock.withLock {
_wrappedValue
}
}
set {
lock.withLock {
_wrappedValue = newValue
}
}
}
}

private final class Lock: LockProtocol {
Copy link
Owner

@michaeleisel michaeleisel Feb 5, 2025

Choose a reason for hiding this comment

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

i think we could remove this class and just have a free function that returns a LockProtocol

let innerLock: LockProtocol

init() {
// Use the lighter-weight `OSAllocatedUnfairLock` if available
if #available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) {
innerLock = OSAllocatedUnfairLock()
} else {
innerLock = NSLock()
}
}

func withLock<T>(_ body: () throws -> T) rethrows -> T {
return try innerLock.withLock(body)
}
}

private protocol LockProtocol {
func withLock<T>(_ body: () throws -> T) rethrows -> T
}

extension NSLock: LockProtocol {
func withLock<T>(_ body: () throws -> T) rethrows -> T {
lock()
defer { unlock() }
return try body()
}
}

@available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *)
extension OSAllocatedUnfairLock: LockProtocol {
func withLock<T>(_ body: () throws -> T) rethrows -> T {
try withLockUnchecked { _ in try body() }
}
}