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

Add background-compatible URLSessionClient class #1163

Merged
merged 12 commits into from
Apr 27, 2020

Conversation

designatednerd
Copy link
Contributor

I WOULD LOVE FEEDBACK ON THIS, LOVE IT OR HATE IT. ESPECIALLY IF YOU HATE IT

I've started noodling around with some major changes to our networking stack and I managed to get one piece done in a way that's relatively straightforward: Making our networking compatible with background URL sessions.

I'm planning to put together a longer RFC on the stuff I'm noodling with (basically: Switching to an interceptor pattern instead of a delegate pattern) in the next week, but this is enough of a drop-in replacement for something we're already using that I think it can go straight in.

In this PR:

  • Added a delegate-based class for URLSession handling to allow things to work in the background (background sessions only work with the delegate-based API because Apple hates us all). This is open and contains open default implementations for methods on all delegates we're implementing so that they can easily be subclassed.
  • BREAKING: Updated the initializer for HTTPNetworkTransport to take the URLSessionClient class rather than a URLSession directly. This is basically because there's no way to set the delegate on a URLSession except in the initializer (I suspect to prevent a session from going from background to foreground).
  • Updated mocking tools to use the new URLSessionClient.
  • Updated instructions for advanced client usage.

A few notes:

  • I've tried to keep this as straightforward as possible so that other changes I want to make to our stack won't have to touch this - it's purely for actually dealing with communicating with the network.
  • This'll likely be annoying for people who are sharing a single session with non-Apollo API classes, but like I said, there's not really a way around it if we can't update the delegate on an existing session.
  • The RawCompletion stuff is basically a workaround to keep our HTTPNetworkTransportTaskCompletedDelegate working while I work on an alternative system. Hopefully the system I'm working on will eliminate the need for that, but it's definitely not something I can just drop completely yet.
  • Note that if you call cancel(task:) on URLSessionClient, you will not receive an error callback that your task was cancelled, since the intent is obviously to cancel. If you really love getting NSURLErrorCancelled, you'll still get that callback if you call cancel() directly on the task.

@amadeu01
Copy link

@designatednerd This background compatibility is related to this API ?

If a URL session finishes its work when your app is not running, the system launches your app in the background so that it can process the event. In that situation, use the provided identifier to create a new URLSessionConfiguration and URLSession object. You must configure the other options of your URLSessionConfiguration object in the same way that you did when you started the uploads or downloads. Upon creating and configuring the new URLSession object, that object calls the appropriate delegate methods to process the events.

If so, should we alert the developers to properly use background data transfer on iOS, they need to override the application(_:handleEventsForBackgroundURLSession:completionHandler:) and call the completionHandler after finish processing the Apollo response? Otherwise, the OS might finish the app before the data process, or the OS could keep the app of requesting in the background after the app died

}

#if os(iOS) || os(tvOS) || os(watchOS)
open func urlSessionDidFinishEvents(forBackgroundURLSession session: URLSession) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we kind of warning the developers that they should call the completionHandler from application(_:handleEventsForBackgroundURLSession:completionHandler:) after this method had been called?
Maybe allow the developer to pass a closure that could be called after the event be triggered?

Something like:

open func urlSessionDidFinishEvents(forBackgroundURLSession session: URLSession) {
    self.sessionDidFinishEvents?()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why this is open - so that it can be subclassed if needed and the developer can handle this as needed.

@designatednerd
Copy link
Contributor Author

@amadeu01 It's related to the fact that even if you use a background URLSession, when you use the completion-block-based API (as we have been doing) you immediately get a crash telling you that you can only use the delegate-based API with it.

I haven't added additional documentation beyond what Apple's documentation is because I want people to read Apple's documentation for this. I'll add a note in the main documentation of the class that people should look at that documentation, though.

@designatednerd
Copy link
Contributor Author

Leaving this open a couple more days for feedback since it's a pretty significant change if you're passing in a URLSession already.

@designatednerd
Copy link
Contributor Author

OK, since this has been open for a week and nobody else has comments, I'm gonna ship it. :shipit: 😇

@designatednerd designatednerd merged commit 95eaacb into master Apr 27, 2020
@designatednerd designatednerd added this to the Next Release milestone Apr 27, 2020
@designatednerd designatednerd deleted the add/urlsessionclient branch April 27, 2020 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants