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

Commit

Permalink
[core] Fix GeoJSONVTData ownership and life cycle
Browse files Browse the repository at this point in the history
Before this change, the `GeoJSONVTData` instance was retained at the scheduled
lambda, which run on the worker thread represented by the `GeoJSONVTData::scheduler`
class member:

```
        std::weak_ptr<GeoJSONVTData> weak = shared_from_this();
        scheduler->scheduleAndReplyValue(
            [id, weak, this]() -> TileFeatures {
                if (auto self = weak.lock()) {
                    return impl.getTile(id.z, id.x, id.y).features;
                }
                return {};
            },
            fn);
```

It caused program termination in case `self` turned to be the last reference to `this`,
as the `std::thread` destructor was called from the thread it represented.

Now, only the `GeoJSONVTData::impl` class member is retained.
  • Loading branch information
pozdnyakov committed Jan 9, 2020
1 parent 236afc0 commit 3ea4592
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 11 deletions.
17 changes: 6 additions & 11 deletions src/mbgl/style/sources/geojson_source_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,12 @@
namespace mbgl {
namespace style {

class GeoJSONVTData : public GeoJSONData, public std::enable_shared_from_this<GeoJSONVTData> {
class GeoJSONVTData : public GeoJSONData {
public:
void getTile(const CanonicalTileID& id, const std::function<void(TileFeatures)>& fn) final {
assert(fn);
std::weak_ptr<GeoJSONVTData> weak = shared_from_this();
scheduler->scheduleAndReplyValue(
[id, weak, this]() -> TileFeatures {
if (auto self = weak.lock()) {
return impl.getTile(id.z, id.x, id.y).features;
}
return {};
},
fn);
[id, impl = this->impl]() -> TileFeatures { return impl->getTile(id.z, id.x, id.y).features; }, fn);
}

Features getChildren(const std::uint32_t) final { return {}; }
Expand All @@ -39,8 +32,10 @@ class GeoJSONVTData : public GeoJSONData, public std::enable_shared_from_this<Ge
private:
friend GeoJSONData;
GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options)
: impl(geoJSON, options), scheduler(Scheduler::GetSequenced()) {}
mapbox::geojsonvt::GeoJSONVT impl;
: impl(std::make_shared<mapbox::geojsonvt::GeoJSONVT>(geoJSON, options)),
scheduler(Scheduler::GetSequenced()) {}

std::shared_ptr<mapbox::geojsonvt::GeoJSONVT> impl; // Accessed on worker thread.
std::shared_ptr<Scheduler> scheduler;
};

Expand Down
1 change: 1 addition & 0 deletions src/mbgl/util/thread_pool.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class ThreadedScheduler : public ThreadedSchedulerBase {
~ThreadedScheduler() override {
terminate();
for (auto& thread : threads) {
assert(std::this_thread::get_id() != thread.get_id());
thread.join();
}
}
Expand Down

0 comments on commit 3ea4592

Please sign in to comment.