Support for Background Sessions as well as SessionDelegate override closures

[cnoon]

Hey @mattt,

I've been working on a library built ontop of Alamofire to handle oauth refresh as well as some really interesting data validation and parsing. Overall we have been extremely pleased with Alamofire. The design is fantastic and it has truly been a pleasure to get to work with it. I can't wait to see how this evolves over the coming months!

The latest feature I've been working on is background session support. When digging into how it should integrate into Alamofire, I've hit some roadblocks. A really great discussion took place a couple months ago in #194 which documents some of the challenges. After reading through that thread multiple times, I started down the path of adding support for background sessions in Alamofire.

The initial problem I ran into was that I could not set the SessionDelegate sessionDidFinishEventsForBackgroundURLSession: closure. I couldn't even get the delegate property on the SessionManager. After digging in deeper, I realized that all the session delegate closures you have added are basically placeholders. None of them are being set to do anything. All the implementation is built into the task delegates. Once I understood the challenges, I decided to crack the nut wide open to "attempt" to understand what your placeholder closures "could" be used for.

Internal Session Delegate

The first thing that had to change was to make the SessionDelegate public along with the SessionManager delegate property. For the future iterations of Alamofire, I'm not sure you can possibly keep this internal. The reason is that by locking down those delegate callbacks, we can't get in to override the default behavior, even if we have to. While I think it is awesome to get so much default behavior for free, it is certainly necessary in certain cases for us to be able to access the session delegate callbacks (background session finishing) through mechanisms other than the response Serializers.

Overriding Session Delegate Closures

The next step was to make all the SessionDelegate closures public as well so we can actually set them. That part was easy, but the more I dug into how those closures were used, the more problematic it became. I realized that the logic for calling those closures was only partially implemented. Not all of the closures were being called in the delegate methods.

What makes the most sense to me is to have the closures be public allowing users to override any of the SessionDelegate methods if they need to. It also seems that the SessionDelegate should always prioritize the closures over the internal Alamofire implementation. This is why I make sure to always call the closure before the internal implementation if it is not nil.

There were also some small bugs in the closure declarations (81c69be, bbf16e1), a missing closure (8b6af2f) and a missing data task delegate call (dfab3f3) that I fixed up.

Background Completion Handler

The last change that I made (582267b) was to actually add a backgroundCompletionHandler closure property to the SessionManager. I'm still a bit on the fence about this change. It's certainly not necessary, but is a nice-to-have. By adding the property, we can automatically set up the SessionDelegate sessionDidFinishEventsForBackgroundURLSession closure to call the handler if it exists.

required public init(configuration: NSURLSessionConfiguration? = nil) {
    self.delegate = SessionDelegate()
    self.session = NSURLSession(configuration: configuration, delegate: delegate, delegateQueue: nil)

    self.delegate.sessionDidFinishEventsForBackgroundURLSession = { [weak self] session in
        if let strongSelf = self {
            strongSelf.backgroundCompletionHandler?()
        }
    }
}

The only issue with the default implementation is that it doesn't give you the chance to handle your events before the handler is called. To do that you'd need to override the sessionDidFinishEventsForBackgroundURLSession closure directly. I think that's what most people will need to do when using background session downloads / uploads. That's why I'm a bit on-the-fence about this change.

Closures in the Session Manager

Another approach to this would be to expose the delegate closures directly in the SessionManager. This would allow you to keep the SessionDelegate internal, yet allow users to override those methods if they need to. This just seems completely wrong though from a design standpoint which is why I didn't go with this approach.

Summary

I'd love to get your thoughts around this approach and to see if I interpreted your placeholder closures correctly. IMO, I love this approach because it exposes the session delegate callbacks while still hiding all the internals of all the task delegates which is where all your default logic resides. I think this strikes a really nice balance. Most users will never have a need to override any of the behavior. In the rare case that you need to and really understand what you're doing, you can.

[hborders]

I need these features as well. I want to be able to change redirect behavior like #314, and I also want to change authentication challenge behavior. This seems like it would enable both of my requirements.

[cnoon]

Hi @hborders, I just pulled the upstream master commits into my background_sessions branch. Feel free to fork it or work off it for now until @mattt gets a chance to review this PR and #350. We've already moved to the fork for an internal library we've been building b/c we really need the background session support as well.

[hborders]

Thanks. I don't need it immediately, but I'll need it in a few months. Just trying to get out ahead of things. Not trying to pressure @mattt, just upvoting the feature.

[mattt]

Thanks so much for your hard work on this, @cnoon. I merged everything into master, and am excited to have this all for the 1.2.0 release. 🍞

[cnoon]

Awesome, thanks @mattt!