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

[WIP] Make the library thread-safe. #620

Merged
merged 15 commits into from
Aug 7, 2017
Merged

[WIP] Make the library thread-safe. #620

merged 15 commits into from
Aug 7, 2017

Conversation

tcard
Copy link
Contributor

@tcard tcard commented Jul 21, 2017

Work in progress! There are still some deadlocks around. The test suite is pretty broken right now.

This change makes the library thread-safe. Users can call all methods and access
all properties from whatever thread they want.

To achieve this, this is the general strategy used:

  • We have a serial queue where all our code runs.
  • To ensure all our code runs in the serial queue, we need to dispatch to it
    from all entry points, namely:
    • User code: all public methods use
    • HTTP requests: all HTTP responses are handled at the same place and dispatched there.
    • Reachability callback from the OS.
    • Events dispatches/timeouts from ARTEventEmitter, which use NSNotificationCenter under the hood: all are dispatched to the queue.
    • WebSocket incoming messages: we set the queue as delegate using SocketRocket's setDelegateQueue.
  • By default, the queue's delegate is the global queue with QOS_CLASS_BACKGROUND. So all our operations will be very low-priority, which I think is the right choice for Ably. The user can change this by specifying ARTClientOptions.internalDispatchQueue.
  • Additionally, callbacks provided from the user are dispatched to another queue, which by default is the main queue. This is a convenience, since they probably want to react to Ably's callbacks by updating the UI, which only can be done from the main queue. This can be overriden too, with ARTClientOptions.dispatchQueue.
  • Public, mutable properties (e. g. connection and channel state) need always to be read and written to from the queue. This is pretty annoying since those reads need to be synchronous, since we're offering a synchronous interface. A synchronous dispatch to a queue is annoying since you can't do it if you're in the queue already, so, from within the queue (ie. from our code) we need to read those properties without the sync dispatch. So those get an additional <property name>_nosync getter. Pretty clumsy but, well, this is Objective-C.
  • Some public methods don't go async right away, since they have some effects that need to be done right after the call, or need to return something. For instance, attach leaves the channel in the ATTACHING state synchronously. Similarly to public, mutable properties as described above, those method get an additional method without the sync dispatch, to use from inside our code.

@tcard tcard requested a review from mattheworiordan July 21, 2017 02:10
@tcard
Copy link
Contributor Author

tcard commented Jul 21, 2017

A side effect I didn't see coming from this is that, since more operations are done asynchronously and the library holds to weak references even less time, #607 is even worse now. Something like this doesn't work:

{
    let rest = ARTRest(...)
    rest.auth.requestToken(...) {
        // Will never be called!
    }
}

Now users will really need to make sure their rest and realtime objects live until after callbacks are called back.

@tcard
Copy link
Contributor Author

tcard commented Aug 2, 2017

The test suite is passing now. ThreadSanitizer is still coughing out issues, though.

@mattheworiordan
Copy link
Member

The test suite is passing now.

🎆

ThreadSanitizer is still coughing out issues, though.

Oh, well at least we're getting the warnings I suppose.

tcard added 15 commits August 4, 2017 19:23
**Work in progress!** There are still some deadlocks around. The test suite is pretty broken right now.

This change makes the library thread-safe. Users can call all methods and access
all properties from whatever thread they want.

To achieve this, this is the general strategy used:

* We have a serial queue where all our code runs.
* To ensure all our code runs in the serial queue, we need to dispatch to it
  from all entry points, namely:
  - User code: all public methods use
  - HTTP requests: all HTTP responses are handled at the same place and dispatched there.
  - Reachability callback from the OS.
  - Events dispatches/timeouts from ARTEventEmitter, which use NSNotificationCenter under the hood: all are dispatched to the queue.
  - WebSocket incoming messages: we set the queue as delegate using SocketRocket's `setDelegateQueue`.
* By default, the queue's delegate is the global queue with QOS_CLASS_BACKGROUND. So all our operations will be very low-priority, which I think is the right choice for Ably. The user can change this by specifying `ARTClientOptions.internalDispatchQueue`.
* Additionally, callbacks provided from the user are dispatched to another queue, which by default is the main queue. This is a convenience, since they probably want to react to Ably's callbacks by updating the UI, which only can be done from the main queue. This can be overriden too, with `ARTClientOptions.dispatchQueue`.
* Public, mutable properties (e. g. connection and channel state) need always to be read and written to from the queue. This is pretty annoying since those reads need to be synchronous, since we're offering a synchronous interface. A synchronous dispatch to a queue is annoying since you can't do it if you're in the queue already, so, from within the queue (ie. from our code) we need to read those properties without the sync dispatch. So those get an additional `<property name>_nosync` getter. Pretty clumsy but, well, this is Objective-C.
* Some public methods don't go async right away, since they have some effects that need to be done right after the call, or need to return something. For instance, `attach` leaves the channel in the ATTACHING state synchronously. Similarly to public, mutable properties as described above, those method get an additional method without the sync dispatch, to use from inside our code.
Don't rely on weak references to the ARTRest.
Exceptions don't play nice with GCD dispatches.
Avoid crashes on test failures.
Use the new waitFor helper instead. Also, use proper type params.
It was just too easy to accidentally cause a deadlock with the main
thread, since waitUntil is probably using it. It's a hack. Better to
avoid it.

Also, improve error reporting, and simplify by using waitFor.
The main thread may be in use, and best to avoid Ably threads.
@tcard
Copy link
Contributor Author

tcard commented Aug 4, 2017

I believe this is finally ready.

@tcard tcard merged commit 57f0e77 into master Aug 7, 2017
@tcard tcard deleted the thread-safe branch August 7, 2017 13:38
@mattheworiordan
Copy link
Member

I believe this is finally ready.

Woop woop. Thanks @tcard

tcard added a commit that referenced this pull request Dec 8, 2019
This was somehow left behind as part of the whole #620 effort.

Fixes #926.
tcard added a commit that referenced this pull request Dec 9, 2019
* Make ARTRealtimeChannel.publish thread-safe.

This was somehow left behind as part of the whole #620 effort.

Fixes #926.

* ARTRealtimeChannel.publish: dispatch_sync instead of async.

Tests rely on this to observe the effects right after the method is
called.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants