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.

Sign up for GitHub

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

Jump to bottom

Dealloc completion blocks on the context they're scheduled on #38

Closed
lilyball opened this issue Apr 2, 2019 · 3 comments
Closed

Dealloc completion blocks on the context they're scheduled on #38

lilyball opened this issue Apr 2, 2019 · 3 comments
Labels
enhancement New feature or request
Milestone
v0.5.1

Comments

@lilyball
Copy link
Owner

lilyball commented Apr 2, 2019

We should at least do this for .main, if not other contexts.

PMHTTP does something similar for its completion blocks. It uses code like

extension HTTPManagerRequestPerformable {
    /// A workaround to ensure the completion block deinits on the queue where it's fired from.
    ///
    /// Ownership of blocks is not passed directly into the queue that runs them, which means they could get
    /// deallocated from the calling thread if the queue runs the block before control returns back to that
    /// calling thread. We guard against that here by forcing the completion block to be released immediately
    /// after it's fired. This ensures that it can only be deallocated on the completion queue or the thread that
    /// creates the task in the first place.
    fileprivate func completionThunk(for block: @escaping (_ task: HTTPManagerTask, _ result: HTTPManagerTaskResult<ResultValue>) -> Void) -> (HTTPManagerTask, HTTPManagerTaskResult<ResultValue>) -> Void {
        var unmanagedThunk: Unmanaged<Thunk<(HTTPManagerTask, HTTPManagerTaskResult<ResultValue>), Void>>? = Unmanaged.passRetained(Thunk(block))
        return { (task, result) in
            let thunk = unmanagedThunk.unsafelyUnwrapped.takeRetainedValue()
            unmanagedThunk = nil // effectively a debug assertion that ensures we don't call the completion block twice
            thunk.block((task, result))
        }
    }
}

/// A wrapper class that allows us to stuff the block into an `Unmanaged`.
private class Thunk<Args,ReturnValue> {
    let block: (Args) -> ReturnValue
    init(_ block: @escaping (Args) -> ReturnValue) {
        self.block = block
    }
}
@lilyball lilyball added the enhancement New feature or request label Apr 2, 2019
@lilyball lilyball added this to the Next milestone Apr 2, 2019
@lilyball
Copy link
Owner Author

lilyball commented Apr 6, 2019

After looking into this, I don't think this is actually doable after all. There's two reasons:

  1. This really only works if we're actually executing the block. If we don't execute the block, then deallocating it on the target context requires executing a block on the target context after all, which in most cases is just wasted work.
  2. It turns out to be rather complicated to do this because there are several levels of blocks involved here and we need to manage the lifetime of the block through all of them. In addition, there are a ton of methods that would need to be updated accordingly, and missing even one of them would reintroduce this issue.

If we cared enough, issue 2 could be dealt with, but issue 1 is a pretty big showstopper. I'm not inclined to be dispatching blocks onto the main queue purely to allow a block to dealloc there unless there's a very good reason to do so.

@lilyball
Copy link
Owner Author

lilyball commented Apr 6, 2019

Thinking about it some more, it's not just wasted work to send these blocks to the queue, but also if this is generalized for all contexts and not just the main context then it can produce seriously bad performance issues when low-priority queues are involved, e.g.

somePromise.catch(on: .background, {
    …
}).then(on: .main, {
    …
})

In this scenario, a successful promise would still bounce through the .background queue before executing the then block, which isn't at all what we want.

Restricting it to just the main context solves this for the most part, unless the main thread is busy with work and we want to run a followup on a background queue. For example:

somePromise.then(on: .main, {
    …
}).catch(on: .userInitiated, {
    …
})

If the main queue is backed up, our catch block will be delayed unnecessarily.

@lilyball
Copy link
Owner Author

lilyball commented Apr 7, 2019

I think I'm going to go ahead with this with the restriction that we only make this guarantee if the block actually executes (or would have executed if any relevant token wasn't invalidated).

This means if you need this behavior, use .always or a .mapResult variant.

We don't just guarantee this for those methods because e.g. a Promise<T,NoError> is guaranteed not to be rejected, and if you can guarantee yourself that it won't be cancelled either then relying on this for then would be fine. Relying on it for catch won't work, but at this point there's no sense in making exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.5.1
Development

No branches or pull requests
1 participant
@lilyball