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

Stop delivering status updates when offline download is canceled #5538

Closed
danswick opened this issue Jul 1, 2016 · 11 comments · Fixed by #6186
Closed

Stop delivering status updates when offline download is canceled #5538

danswick opened this issue Jul 1, 2016 · 11 comments · Fixed by #6186
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS offline

Comments

@danswick
Copy link
Contributor

danswick commented Jul 1, 2016

Platform: iOS and Android
Mapbox SDK version: latest of each

Steps to trigger behavior

  1. Initiate an offline download
  2. Cancel the download

Expected behavior

Download immediately stops and no further messages are sent.

Actual behavior

Messages continue to be sent.

Related issues: #5531, #4418, #4229

@danswick danswick added iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android offline labels Jul 1, 2016
@1ec5
Copy link
Contributor

1ec5 commented Jul 1, 2016

Should the region (pack) go back to reporting its state as inactive? Is it safe for the developer to resume downloading the region at that point, before region would’ve stopped updating (with the current code)?

@zugaldia
Copy link
Member

zugaldia commented Jul 1, 2016

/sub

@dagatsoin
Copy link

Just to be sure is

public void cancelDownload(){
        _offlineRegion.setDownloadState(OfflineRegion.STATE_INACTIVE);
    }

the good way to cancel a download ?

@zugaldia
Copy link
Member

Android has enabled an API to achieve this via #5696. Setting OfflineRegion.setDeliverInactiveMessages(true/false) will let you enable/disable the delivery of messages once a region has been set inactive.

@1ec5 1ec5 added Core The cross-platform C++ core, aka mbgl and removed Android Mapbox Maps SDK for Android iOS Mapbox Maps SDK for iOS labels Jul 14, 2016
@1ec5
Copy link
Contributor

1ec5 commented Jul 14, 2016

We’re leaving this ticket open to track a fix in mbgl that would obviate the API added in #5696.

@madsodgaard
Copy link

Any update on iOS ETA?

@1ec5
Copy link
Contributor

1ec5 commented Jul 26, 2016

There are no plans to add a dedicated API to work around this issue on iOS and macOS. Instead, this ticket is tracking a fix in core that would stop sending these notifications when appropriate. We consider it essentially a bug in core that the state is still reported as active and notifications keep coming in.

In the meantime, if you absolutely need the cancelation to appear to take effect immediately, the workaround on iOS and macOS is to keep track of the packs you've canceled or keep track of the fact that this particular pack has been canceled, and ignore progress notifications based on that.

/cc @jfirebaugh

@1ec5
Copy link
Contributor

1ec5 commented Aug 19, 2016

One consequence of this issue is that progress updates may be coming in after the developer has attempted to remove the offline pack. The errant progress updates would lead to bad memory access, as seen in #6092. The approach in #5696 wouldn’t address this race condition on iOS and macOS, because the MGLOfflinePack object has already been reclaimed by ARC by this point.

@jfirebaugh
Copy link
Contributor

It's not possible to fix this at the core level. The status updates that to the main thread appear to come after the request to cancel a download were in fact sent by the asynchronous worker before it processed the request to cancel the download.

iOS and macOS should apply a fix similar to that applied on Android, where the main thread ignores notifications delivered after the request to cancel the download has been sent to the asynchronous worker.

@jfirebaugh jfirebaugh added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS and removed Core The cross-platform C++ core, aka mbgl labels Aug 27, 2016
@1ec5
Copy link
Contributor

1ec5 commented Aug 27, 2016

In that case, I don’t think it makes sense to add a dedicated “deliverInactiveMessages” property, as #5696 did, to the iOS/macOS SDK. The SDK should just do the right thing, which apparently means dropping these notifications.

@jfirebaugh
Copy link
Contributor

#5508 is also related.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS offline
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants