Websocket Data Race when stream connection fails

apollographql / apollo-ios

📱 A strongly-typed, caching GraphQL client for iOS, written in Swift.

https://www.apollographql.com/docs/ios/

MIT License

3.89k stars 730 forks source link

Websocket Data Race when stream connection fails #3466

Closed aaronbarsky closed 3 weeks ago

aaronbarsky commented 3 weeks ago

Summary

When stream connection fails triggering doDisconnect, the code to set the callback queue runs concurrently with doDisconnect which reads the callback queue.

I believe this can be resolved, by moving the assignment of the callback queue before the connect call (line 173 to 170).

Version

1.13.1

Steps to reproduce the behavior

It's a race condition, so it's hard to reproduce. Discovered by running Xcode with the Thread Sanitizer enabled.

Logs

No response

Anything else?

No response

calvincestari commented 3 weeks ago

Thanks @aaronbarsky. I can the see potential for the data race here so I've made the change in PR #529. To be honest it makes sense to assign the given queue as early as possible, I'm not sure why it was in that order before.

github-actions[bot] commented 3 weeks ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

calvincestari commented 3 weeks ago

@aaronbarsky, this change is merged and will go out with the next release.