Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios][macos] Add MGLNetworkConfiguration.sessionDelegate #15128

Closed
wants to merge 3 commits into from

Conversation

halset
Copy link
Contributor

@halset halset commented Jul 16, 2019

This is an attempt to solve #11888 by expanding MGLNetworkConfiguration with a property to set a custom NSURLSessionDelegate.

My main motivation for doing this is to be able to use tile services that uses HTTP Basic Authentication.

This is a new version of #13491 as that was made before MGLNetworkConfiguration went public.

@halset halset requested a review from 1ec5 as a code owner July 16, 2019 13:30
@halset halset requested a review from a team July 16, 2019 13:30
id<NSURLSessionDelegate> sessionDelegate =
[MGLNetworkConfiguration sharedManager].sessionDelegate;

session = [NSURLSession sessionWithConfiguration:sessionConfig delegate:sessionDelegate delegateQueue:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

Since sessionWithConfiguration:delegate:delegateQueue: keeps a strong reference to the delegate, I think we'll need to call invalidateAndCancel or finishTasksAndInvalidate in the destructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I can add a destructor to HTTPFileSource::Impl and call invalidateAndCancel on theNSURLSession there, but how can I test it? Is there a way to tear down the stack? I have tried to stop the app, but the destructor does not seem to be called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for the delay in responding - I believe we'll need to create a test for this. There are cpp tests for HTTPFileSource and OnlineFileSource which we might be able to take advantage of, but I think we'll need to create an integration test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @tmpsantos for an interesting testing scenario (platform <-> core).

Copy link
Contributor

Choose a reason for hiding this comment

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

Added: #15592

@julianrex
Copy link
Contributor

Thanks for this PR @halset - we will discuss and review!

/cc @chloekraw @zugaldia

@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Jul 16, 2019
@halset
Copy link
Contributor Author

halset commented Aug 6, 2019

@julianrex There is a merge-conflict in CHANGELOG.md, but I will await your discussion/review.

@hectorddmx
Copy link

Hi @halset, one question, would this help to recreate the session with a new configuration?
Currently we're using bearer tokens in iOS, which expire in 15 minutes, and I'm able to call vector tiles from my custom endpoint by setting MGLNetworkConfiguration.sharedManager.sessionConfiguration.httpAdditionalHeaders = ["accessToken": newToken]

But after the token is refreshed, I'm getting auth issues since I'm not able to recreate the session with a new configuration.

@julianrex
Copy link
Contributor

Requires #15592

@julianrex julianrex added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold needs tests labels Sep 10, 2019
@patrickkempff
Copy link

any news on this, or how to set a custom header to the request done by mapbox? On Android it is easy, on iOS somehow not..

@julianrex
Copy link
Contributor

This PR is still valid, but needs to be renewed/reviewed in light of recent changes.

/cc @jmkiley

@julianrex
Copy link
Contributor

Closing in favour of mapbox/mapbox-gl-native-ios#228

@julianrex julianrex closed this Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants