search

urbanairship / ios-library

Urban Airship iOS SDK
http://urbanairship.com
Apache License 2.0
478 stars 265 forks source link

Stop using instance based encoder #408

Closed danibachar closed 4 months ago

danibachar commented 4 months ago

What do these changes do?

The changes removes the usage of an instance base encoder and generates a new one for each request under the ChannelAPIClient

Why are these changes necessary?

While its not documented very well, JSONEncoder/Decoder are a stateful objects, which makes them non-thread safe. We are using version 17.10.0 and facing exceptions around that code, which made me think its a thread issue. In anyway spinning a new encoder/decoder is the recommended way and very cheap in resources.

How did you verify these changes?

Testing for thread safety issues is a hard task, as such I could not create a test to reproduce it. Here is a thread supporting my theory - https://forums.swift.org/t/is-it-ok-to-create-jsondecoder-only-once-in-withthrowingtaskgroup/57260

Verification Screenshots:

Anything else a reviewer should know?

Here is the stack trace at the app level crash

Crashed in non-app Foundation | +0x04e7b0 | String.withUTF8<T>
Foundation | +0x04e798 | String.withUTF8<T>
Foundation | +0x04eb88 | JSONWriter.serializeString
Foundation | +0x04e80c | JSONWriter.serializeJSON
Foundation | +0x04cfd0 | JSONWriter.serializeObject
Foundation | +0x04e910 | JSONWriter.serializeJSON
Foundation | +0x04cfd0 | JSONWriter.serializeObject
Foundation | +0x04e910 | JSONWriter.serializeJSON
Foundation | +0x0d06f8 | JSONEncoder.encode<T>
Foundation | +0x0d0480 | JSONEncoder.encode<T>
MyAppName | +0xf8a454 | ChannelAPIClient.updateChannel (ChannelAPIClient.swift:105) (In App)
Called from libswift_Concurrency | +0x04d760 | swift::runJobInEstablishedExecutorContext
crow commented 4 months ago

@danibachar Looks like we'll need to comb through the SDK and create instances of JSON coders at the site(s) of use in a couple different places. I've made a ticket to track the work and will ensure this fix makes it into one of our next releases in the short term. Thanks for tracking this down. I'll update you when the change is in.

danibachar commented 4 months ago

Done - https://github.com/urbanairship/ios-library/pull/410/files CC @crow

rlepinski commented 4 months ago

Thanks!