Make ZippyJSONDecoder thread-safe and conform to Sendable

[JaviSoto]

No description provided.

[JaviSoto]

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

[michaeleisel]

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

[JaviSoto]

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

[michaeleisel]

very nice!

[michaeleisel]

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

[michaeleisel]

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

[michaeleisel]

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

[michaeleisel]

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

[michaeleisel]

thank you for this pr. i should mention that JSONDecoder is superior to ZippyJSON for iOS 17+, as discussed here: #69 . things may get archived in, say, a year. so if you're doing this change in part for your own code base, i'm curious to know if it supports ios 16 or prior

although this pr makes sense with what it's trying to do, i have a high level concern. if someone, say, changes the data decoding strategy of a decoder on thread A while thread B is doing json decoding with it, then although the changeover will be safe, the results are probably undesired. data decoding would behave differently for the first part of the json document than the second. i don't know what the best solution is here, but i'm not sure if people should actually be using it from multiple threads here (it should be noted i haven't done a ton with swift since Sendable came out)

[JaviSoto]

thank you for this pr. i should mention that JSONDecoder is superior to ZippyJSON for iOS 17+, as discussed here: #69 . things may get archived in, say, a year. so if you're doing this change in part for your own code base, i'm curious to know if it supports ios 16 or prior

Hey! Thanks for that :) I was actually just in the process of doing benchmarks in my app. But that is good to hear! May be worth updating the README :)

changes the data decoding strategy of a decoder on thread A while thread B is doing json decoding with it, then although the changeover will be safe, the results are probably undesired. data decoding would behave differently for the first part of the json document than the second. i don't know what the best solution is here, but i'm not sure if people should actually be using it from multiple threads here (it should be noted i haven't done a ton with swift since Sendable came out)

yeah totally. I wonder what JSONDecoder does. It's marked @unchecked Sendable. To me it would make more sense as a struct which would avoid that issue altogether.

I'm happy to just close this PR though!

[michaeleisel]

Hey! Thanks for that :) I was actually just in the process of doing benchmarks in my app. But that is good to hear! May be worth updating the README :)

Sure, that's a good idea, I'll update it.

yeah totally. I wonder what JSONDecoder does. It's marked @unchecked Sendable. To me it would make more sense as a struct which would avoid that issue altogether.

It looks like this:

    open var nonConformingFloatDecodingStrategy: NonConformingFloatDecodingStrategy {
        get {
            optionsLock.lock()
            defer { optionsLock.unlock() }
            return options.nonConformingFloatDecodingStrategy
        }
        _modify {
            optionsLock.lock()
            var value = options.nonConformingFloatDecodingStrategy
            defer {
                options.nonConformingFloatDecodingStrategy = value
                optionsLock.unlock()
            }
            yield &value
        }
        set {
            optionsLock.lock()
            defer { optionsLock.unlock() }
            options.nonConformingFloatDecodingStrategy = newValue
        }
    }

So I guess they use a shared lock. It has that fancy _modify which I guess is after my time. They're modifying a struct options, and based on JSONDecoderImpl(userInfo: self.userInfo, from: map, codingPathNode: .root, options: self.options), they keep a separate struct for each parsing. So it looks like they've solved the higher level issue.

If you want to solve the high level problem and then mark it Sendable, or if you want to merge your current changes minus marking it Sendable, or of course abandon the PR, all are things I'm on board with