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

Resume file source to complete resources cache path change #14546

Merged
merged 2 commits into from
May 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion include/mbgl/storage/default_file_source.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ template <typename T> class Thread;

class ResourceTransform;

// TODO: the callback should include a potential error info when https://github.com/mapbox/mapbox-gl-native/issues/14759 is resolved
using PathChangeCallback = std::function<void ()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a way to communicate failure?

Right now, void OfflineDatabase::changePath(const std::string&) can throw exception, should developer be notified when setResourceCachePath fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the way for void OfflineDatabase::changePath(const std::string&) to throw? Is that only the assert(!db) and assert(statements.empty()) assertions in OfflineDatabase::initialize()? If that's the case, we are logging a failure to cleanup beforehand, so I don't think we need to send failure callback just before we crash anyway. Same if we fail to open the database.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the underlying problem here is that we don't have a proper mechanism for detecting errors when we failed to open the database/

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this something that we'd be able to follow up on in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukasPaczos almost every line in OfflineDatabase::initialize throws an exception (db->*, file deletion). ResourcesCachePathChangeCallback already provides onError, that might be used on SDK side to notify developer when changePath failed.

This PR did not introduce that issue, so, I think fix for propagating an error on initialize can be done in separate PR. May be worth creating issue + adding TODO note for PathChangeCallback that refers to an issue.

@tmpsantos wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #14759 and added a TODO.


class DefaultFileSource : public FileSource {
public:
/*
Expand Down Expand Up @@ -47,7 +50,7 @@ class DefaultFileSource : public FileSource {

void setResourceTransform(optional<ActorRef<ResourceTransform>>&&);

void setResourceCachePath(const std::string&);
void setResourceCachePath(const std::string&, optional<ActorRef<PathChangeCallback>>&&);

std::unique_ptr<AsyncRequest> request(const Resource&, Callback) override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,7 @@ public void onWritePermissionGranted() {
Context.MODE_PRIVATE).edit();
editor.putString(MAPBOX_SHARED_PREFERENCE_RESOURCES_CACHE_PATH, path);
editor.apply();
setResourcesCachePath(applicationContext, path);
callback.onSuccess(path);
internalSetResourcesCachePath(applicationContext, path, callback);
}

@Override
Expand All @@ -326,11 +325,26 @@ public void onError() {
}
}

private static void setResourcesCachePath(@NonNull Context context, @NonNull String path) {
resourcesCachePathLoaderLock.lock();
resourcesCachePath = path;
resourcesCachePathLoaderLock.unlock();
getInstance(context).setResourceCachePath(path);
private static void internalSetResourcesCachePath(@NonNull Context context, @NonNull String path,
@NonNull final ResourcesCachePathChangeCallback callback) {
final FileSource fileSource = getInstance(context);
fileSource.setResourceCachePath(path, new ResourcesCachePathChangeCallback() {
@Override
public void onSuccess(@NonNull String path) {
fileSource.deactivate();
resourcesCachePathLoaderLock.lock();
resourcesCachePath = path;
resourcesCachePathLoaderLock.unlock();
callback.onSuccess(path);
}

@Override
public void onError(@NonNull String message) {
fileSource.deactivate();
callback.onError(message);
}
});
fileSource.activate();
}

private static boolean isPathWritable(String path) {
Expand Down Expand Up @@ -388,7 +402,7 @@ private FileSource(String cachePath, AssetManager assetManager) {
public native void setResourceTransform(final ResourceTransformCallback callback);

@Keep
private native void setResourceCachePath(String path);
private native void setResourceCachePath(String path, ResourcesCachePathChangeCallback callback);

@Keep
private native void initialize(String accessToken, String cachePath, AssetManager assetManager);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.mapbox.mapboxsdk.testapp.storage

import android.os.Handler
import android.support.test.annotation.UiThreadTest
import android.support.test.rule.ActivityTestRule
import android.support.test.runner.AndroidJUnit4
Expand All @@ -10,6 +9,7 @@ import org.junit.*
import org.junit.rules.TestName
import org.junit.runner.RunWith
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit

@RunWith(AndroidJUnit4::class)
class FileSourceStandaloneTest {
Expand Down Expand Up @@ -55,19 +55,57 @@ class FileSourceStandaloneTest {
fileSourceTestUtils.changePath(fileSourceTestUtils.testPath)
Assert.assertEquals(fileSourceTestUtils.testPath, FileSource.getResourcesCachePath(rule.activity))

// workaround for https://github.com/mapbox/mapbox-gl-native/issues/14334
val latch = CountDownLatch(1)
fileSourceTestUtils.changePath(fileSourceTestUtils.originalPath)
Assert.assertEquals(fileSourceTestUtils.originalPath, FileSource.getResourcesCachePath(rule.activity))
}

@Test
fun overridePathChangeCallTest() {
val firstLatch = CountDownLatch(1)
val secondLatch = CountDownLatch(1)
rule.activity.runOnUiThread {
fileSource.activate()
Handler().postDelayed({
FileSource.setResourcesCachePath(
fileSourceTestUtils.testPath,
object : FileSource.ResourcesCachePathChangeCallback {
override fun onSuccess(path: String) {
Assert.assertEquals(fileSourceTestUtils.testPath, path)
firstLatch.countDown()
}

override fun onError(message: String) {
Assert.fail("First attempt should succeed.")
}
})

FileSource.setResourcesCachePath(
fileSourceTestUtils.testPath2,
object : FileSource.ResourcesCachePathChangeCallback {
override fun onSuccess(path: String) {
Assert.fail("Second attempt should fail because first one is in progress.")
}

override fun onError(message: String) {
Assert.assertEquals("Another resources cache path change is in progress", message)
secondLatch.countDown()
}
})
}

if (!secondLatch.await(5, TimeUnit.SECONDS)) {
rule.runOnUiThread {
// if we fail to call a callback, the file source is not going to be deactivated
fileSource.deactivate()
latch.countDown()
}, 2000)
}
Assert.fail("Second attempt should fail.")
}
latch.await()

fileSourceTestUtils.changePath(fileSourceTestUtils.originalPath)
Assert.assertEquals(fileSourceTestUtils.originalPath, FileSource.getResourcesCachePath(rule.activity))
if (!firstLatch.await(5, TimeUnit.SECONDS)) {
rule.runOnUiThread {
// if we fail to call a callback, the file source is not going to be deactivated
fileSource.deactivate()
}
Assert.fail("First attempt should succeed.")
}
}

@After
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,15 @@ import java.util.concurrent.CountDownLatch
class FileSourceTestUtils(private val activity: Activity) {
val originalPath = FileSource.getResourcesCachePath(activity)
val testPath = "$originalPath/test"
val testPath2 = "$originalPath/test2"

private val paths = listOf(testPath, testPath2)

fun setup() {
val testFile = File(testPath)
testFile.mkdirs()
for (path in paths) {
val testFile = File(path)
testFile.mkdirs()
}
}

@WorkerThread
Expand All @@ -22,25 +27,29 @@ class FileSourceTestUtils(private val activity: Activity) {
if (currentPath != originalPath) {
changePath(originalPath)
}
val testFile = File(testPath)
if (testFile.exists()) {
testFile.deleteRecursively()

for (path in paths) {
val testFile = File(path)
if (testFile.exists()) {
testFile.deleteRecursively()
}
}
}

@WorkerThread
fun changePath(path: String) {
fun changePath(requestedPath: String) {
val latch = CountDownLatch(1)
activity.runOnUiThread {
FileSource.setResourcesCachePath(
path,
requestedPath,
object : FileSource.ResourcesCachePathChangeCallback {
override fun onSuccess(path: String) {
Assert.assertEquals(requestedPath, path)
latch.countDown()
}

override fun onError(message: String) {
Assert.fail("Resource path change failed - path: $path, message: $message")
Assert.fail("Resource path change failed - path: $requestedPath, message: $message")
}
})
}
Expand Down
42 changes: 39 additions & 3 deletions platform/android/src/file_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,25 @@ void FileSource::setResourceTransform(jni::JNIEnv& env, const jni::Object<FileSo
}
}

void FileSource::setResourceCachePath(jni::JNIEnv& env, const jni::String& path) {
void FileSource::setResourceCachePath(jni::JNIEnv& env, const jni::String& path,
const jni::Object<FileSource::ResourcesCachePathChangeCallback>& _callback) {
if (pathChangeCallback) {
FileSource::ResourcesCachePathChangeCallback::onError(env, _callback, jni::Make<jni::String>(env, "Another resources cache path change is in progress"));
return;
}

std::string newPath = jni::Make<std::string>(env, path);
mapbox::sqlite::setTempPath(newPath);
fileSource->setResourceCachePath(newPath + DATABASE_FILE);

auto global = jni::NewGlobal<jni::EnvAttachingDeleter>(env, _callback);
pathChangeCallback = std::make_unique<Actor<PathChangeCallback>>(*Scheduler::GetCurrent(),
[this, callback = std::make_shared<decltype(global)>(std::move(global)), newPath] {
android::UniqueEnv _env = android::AttachEnv();
FileSource::ResourcesCachePathChangeCallback::onSuccess(*_env, *callback, jni::Make<jni::String>(*_env, newPath));
pathChangeCallback.reset();
});

fileSource->setResourceCachePath(newPath + DATABASE_FILE, pathChangeCallback->self());
}

void FileSource::resume(jni::JNIEnv&) {
Expand Down Expand Up @@ -123,11 +138,32 @@ mbgl::ResourceOptions FileSource::getSharedResourceOptions(jni::JNIEnv& env, con
return fileSource->resourceOptions.clone();
}

// FileSource::ResourcesCachePathChangeCallback //

void FileSource::ResourcesCachePathChangeCallback::onSuccess(jni::JNIEnv& env,
const jni::Object<FileSource::ResourcesCachePathChangeCallback>& callback,
const jni::String& path) {
static auto& javaClass = jni::Class<FileSource::ResourcesCachePathChangeCallback>::Singleton(env);
static auto method = javaClass.GetMethod<void (jni::String)>(env, "onSuccess");

callback.Call(env, method, path);
}

void FileSource::ResourcesCachePathChangeCallback::onError(jni::JNIEnv& env,
const jni::Object<FileSource::ResourcesCachePathChangeCallback>& callback,
const jni::String& message) {
static auto& javaClass = jni::Class<FileSource::ResourcesCachePathChangeCallback>::Singleton(env);
static auto method = javaClass.GetMethod<void (jni::String)>(env, "onError");

callback.Call(env, method, message);
}

void FileSource::registerNative(jni::JNIEnv& env) {
// Ensure the class for ResourceTransformCallback is cached. If it's requested for the
// Ensure the classes are cached. If they're requested for the
// first time on a background thread, Android's class loader heuristics will fail.
// https://developer.android.com/training/articles/perf-jni#faq_FindClass
jni::Class<ResourceTransformCallback>::Singleton(env);
jni::Class<ResourcesCachePathChangeCallback>::Singleton(env);

static auto& javaClass = jni::Class<FileSource>::Singleton(env);

Expand Down
16 changes: 15 additions & 1 deletion platform/android/src/file_source.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace mbgl {

template <typename T> class Actor;
class ResourceTransform;
using mbgl::PathChangeCallback;

namespace android {

Expand All @@ -28,6 +29,18 @@ class FileSource {
static std::string onURL(jni::JNIEnv&, const jni::Object<FileSource::ResourceTransformCallback>&, int, std::string);
};

struct ResourcesCachePathChangeCallback {
static constexpr auto Name() { return "com/mapbox/mapboxsdk/storage/FileSource$ResourcesCachePathChangeCallback";}

static void onSuccess(jni::JNIEnv&,
const jni::Object<FileSource::ResourcesCachePathChangeCallback>&,
const jni::String&);

static void onError(jni::JNIEnv&,
const jni::Object<FileSource::ResourcesCachePathChangeCallback>&,
const jni::String&);
};

FileSource(jni::JNIEnv&, const jni::String&, const jni::String&, const jni::Object<AssetManager>&);

~FileSource();
Expand All @@ -40,7 +53,7 @@ class FileSource {

void setResourceTransform(jni::JNIEnv&, const jni::Object<FileSource::ResourceTransformCallback>&);

void setResourceCachePath(jni::JNIEnv&, const jni::String&);
void setResourceCachePath(jni::JNIEnv&, const jni::String&, const jni::Object<FileSource::ResourcesCachePathChangeCallback>&);

void resume(jni::JNIEnv&);

Expand All @@ -59,6 +72,7 @@ class FileSource {
optional<int> activationCounter;
mbgl::ResourceOptions resourceOptions;
std::unique_ptr<Actor<ResourceTransform>> resourceTransform;
std::unique_ptr<Actor<PathChangeCallback>> pathChangeCallback;
std::shared_ptr<mbgl::DefaultFileSource> fileSource;
};

Expand Down
9 changes: 6 additions & 3 deletions platform/default/src/mbgl/storage/default_file_source.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,11 @@ class DefaultFileSource::Impl {
onlineFileSource.setResourceTransform(std::move(transform));
}

void setResourceCachePath(const std::string& path) {
void setResourceCachePath(const std::string& path, optional<ActorRef<PathChangeCallback>>&& callback) {
offlineDatabase->changePath(path);
if (callback) {
callback->invoke(&PathChangeCallback::operator());
}
}

void listRegions(std::function<void (expected<OfflineRegions, std::exception_ptr>)> callback) {
Expand Down Expand Up @@ -252,8 +255,8 @@ void DefaultFileSource::setResourceTransform(optional<ActorRef<ResourceTransform
impl->actor().invoke(&Impl::setResourceTransform, std::move(transform));
}

void DefaultFileSource::setResourceCachePath(const std::string& path) {
impl->actor().invoke(&Impl::setResourceCachePath, path);
void DefaultFileSource::setResourceCachePath(const std::string& path, optional<ActorRef<PathChangeCallback>>&& callback) {
impl->actor().invoke(&Impl::setResourceCachePath, path, std::move(callback));
}

std::unique_ptr<AsyncRequest> DefaultFileSource::request(const Resource& resource, Callback callback) {
Expand Down
12 changes: 12 additions & 0 deletions test/storage/default_file_source.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,18 @@ TEST(DefaultFileSource, TEST_REQUIRES_SERVER(SetResourceTransform)) {
loop.run();
}

TEST(DefaultFileSource, SetResourceCachePath) {
util::RunLoop loop;
DefaultFileSource fs(":memory:", ".");

Actor<PathChangeCallback> callback(loop, [&]() -> void {
loop.stop();
});

fs.setResourceCachePath("./new_offline.db", callback.self());
loop.run();
}

// Test that a stale cache file that has must-revalidate set will trigger a response.
TEST(DefaultFileSource, TEST_REQUIRES_SERVER(RespondToStaleMustRevalidate)) {
util::RunLoop loop;
Expand Down