Skip to content

URLInstrumentation: only set a Task Delegate if there is no Session Delegate #747

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MustafaHaddara
Copy link

Fixes #744

Majority of the changes are in test/example code. The actual fix is quite small, but relies on calling task.value(forKey: "session") as? URLSession to get the associated session on a task.

Alternatives

Intercept Task Creation

One other approach we discussed in the sig was to intercept the task creation and call objc_setAssociatedObject on the task, if the session had an active delegate. We already do this here:

if session.configuration.identifier == nil {
objc_setAssociatedObject(task, "IsBackground", true, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
}
return task

However, that blog of code isn't swizzling the URLSession.data(for:) or URLSession.data(from:) functions, which are used to create asynchronous tasks. We could start swizzling those functions as well, if we wanted to, but that felt like a riskier change.

Swizzle UISession

Honeycomb has an alternate URL instrumentation implementation that swizzles the URLSession and URLSessionTask constructors directly. This lets us get a much stronger handle on the sessions and tasks in a wider variety of cases. Switching to that approach would be more costly and time consuming, but may serve us in the long run since the implementation is simpler.

@ArielDemarco
Copy link
Contributor

The PR looks good to me!

The only thing I’d recommend is adding a few tests to cover the different NSURLSessionTask types that can be created (at least the ones this instrumentation handles). I think it’s especially worth testing because the solution relies on KVO of a variable that should exist across the many different NSURLSessionTask subclasses, and the test will help ensure nothing breaks across OS versions as time goes by.

That said, the PR is failing due to the examples added. It seems like there's a simple fix needed to wrap some API usages with @available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL Instrumentation prevents Session Delegates from being called in async contexts
2 participants