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

Shutting down thread pool of the CustomGeometrySource when the source is destroyed #12517

Merged
merged 1 commit into from
Aug 10, 2018
Merged

Shutting down thread pool of the CustomGeometrySource when the source is destroyed #12517

merged 1 commit into from
Aug 10, 2018

Conversation

LukasPaczos
Copy link
Member

Fixes #12516.

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Jul 31, 2018
@LukasPaczos LukasPaczos added this to the android-v6.4.0 milestone Jul 31, 2018
asheemmamoowala
asheemmamoowala suggested changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

When the Android app is sent to the background, is there a notification that can be used to also pause, cancel requests or possible delete these threads ?

platform/android/src/style/sources/custom_geometry_source.cpp

static auto releaseThreads = javaClass.GetMethod<void ()>(*_env, "releaseThreads");

assert(javaPeer);

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved to #12551

@LukasPaczos LukasPaczos force-pushed the 12516-cgs-leaks-threads branch 2 times, most recently from 0453ce2 to 5d3cd21 Compare July 31, 2018 20:03
@LukasPaczos
Copy link
Member Author

Another round for you @asheemmamoowala with 5d3cd21. I'm releasing the threads when the source is removed. Is the current check for javaPeer correct?

Also, 5d3cd21 seems to crash for me when running threadsShutdownWhenSourceRemoved test on arm64-v8a but works just fine on armeabi-v7a. Do you spot anything wrong in the implementation?

What's next to be done is releasing/stopping threads with Android lifecycle.

@LukasPaczos LukasPaczos force-pushed the 12516-cgs-leaks-threads branch 2 times, most recently from 331b77d to b143957 Compare August 1, 2018 12:19
@LukasPaczos
Copy link
Member Author

LukasPaczos commented Aug 1, 2018 •
edited
Loading

b143957 resolves the crash mentioned before that was partially caused by #12526 - the source wasn't removed and tile requests kept coming, even though the executor was killed in a failed attempt to remove the source, resulting in a crash.

@LukasPaczos LukasPaczos force-pushed the 12516-cgs-leaks-threads branch 2 times, most recently from 800ab9b to 157faaf Compare August 1, 2018 14:27
@LukasPaczos
Copy link
Member Author

Trying to shutdown the executor before we release the peer in 157faaf but with that setup I'm hitting

custom_geometry_source.cpp:66: void mbgl::android::CustomGeometrySource::fetchTile(const mbgl::CanonicalTileID &): assertion "javaPeer" failed

which looks like the fetchTile is called after the source is removed.

asheemmamoowala
asheemmamoowala reviewed Aug 2, 2018
View reviewed changes
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

The source is removed on the main thread, but it still exists on the Render thread until the frame is complete and the next style diff is processed there.

custom_geometry_source.cpp:66: void mbgl::android::CustomGeometrySource::fetchTile(const mbgl::CanonicalTileID &): assertion "javaPeer" failed

It's possible this crash existed even before your changes. Can you run the test without any changes and see if you still hit this assert?

platform/android/src/style/sources/custom_geometry_source.hpp Outdated
@@ -28,8 +28,11 @@ class CustomGeometrySource : public Source {

~CustomGeometrySource();

bool removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map&);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add override declaration

platform/android/src/style/sources/source.cpp Outdated

void Source::releaseJavaPeer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are separating this method from removeFromMap, it would be good to add back the checks for valid ownedSource so that it doesn't crash in the apps if called incorrectly.

@LukasPaczos LukasPaczos force-pushed the 12516-cgs-leaks-threads branch 2 times, most recently from c0cd5d7 to 8e99fdf Compare August 6, 2018 12:40
@LukasPaczos LukasPaczos mentioned this pull request Aug 6, 2018
Open
@LukasPaczos
Copy link
Member Author

You are right, the assertion fails on master as well, tracking it in #12551. For the time being I'm adding a delay.

Other comments have been addressed.

@LukasPaczos
Copy link
Member Author

41f3c33 recreates the executor when the previously removed source is re-added.
@asheemmamoowala, would you mind giving this one another look?

@LukasPaczos LukasPaczos merged commit 63d4ac0 into master Aug 10, 2018
@LukasPaczos LukasPaczos deleted the 12516-cgs-leaks-threads branch August 10, 2018 16:31
@LukasPaczos LukasPaczos removed this from the android-v6.4.0 milestone Aug 15, 2018
@LukasPaczos LukasPaczos mentioned this pull request Aug 16, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asheemmamoowala asheemmamoowala asheemmamoowala approved these changes

Assignees
No one assigned
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@LukasPaczos @asheemmamoowala