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

Stop using instance based encoder #408

Merged

Conversation

danibachar
Copy link
Contributor

@danibachar danibachar commented Jul 22, 2024

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
Copy link
Contributor

crow commented Jul 23, 2024

@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
Copy link
Contributor Author

danibachar commented Jul 23, 2024

@rlepinski rlepinski merged commit daabdb6 into urbanairship:main Jul 24, 2024
12 checks passed
@rlepinski
Copy link
Contributor

Thanks!

@danibachar danibachar deleted the danibachar/create-encoder-on-the-fly branch July 25, 2024 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants