search

urbanairship / ios-library

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

Remove Stateful JSON encoding and decoding #410

Closed danibachar closed 4 months ago

danibachar commented 4 months ago

This PR is in response to this one, converting all instance based usage of encoders/decoder

What do these changes do?

The changes removes the usage of an instance base encoders/decoders

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 Foundation | +0x04e798 | String.withUTF8 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 Foundation | +0x0d0480 | JSONEncoder.encode MyAppName | +0xf8a454 | ChannelAPIClient.updateChannel (ChannelAPIClient.swift:105) (In App) Called from libswift_Concurrency | +0x04d760 | swift::runJobInEstablishedExecutorContext