From 780e5959331b39eda19868126af41398a904818b Mon Sep 17 00:00:00 2001 From: frosch Date: Sat, 7 Mar 2015 18:27:01 +0000 Subject: (svn r27178) -Fix [FS#5969]: Data race due to lazy initialisation of objects. --- src/cheat_gui.cpp | 2 +- src/linkgraph/linkgraphjob.cpp | 7 +++++++ src/linkgraph/linkgraphjob.h | 2 ++ src/linkgraph/linkgraphschedule.cpp | 32 ++++++++++++++------------------ src/linkgraph/linkgraphschedule.h | 2 +- src/linkgraph/mcf.cpp | 10 ++++------ src/saveload/linkgraph_sl.cpp | 8 ++++---- src/station.cpp | 2 +- src/station_base.h | 2 ++ src/station_cmd.cpp | 15 +++++++++++---- src/toolbar_gui.cpp | 2 +- 11 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/cheat_gui.cpp b/src/cheat_gui.cpp index e252912c3..fda2b246e 100644 --- a/src/cheat_gui.cpp +++ b/src/cheat_gui.cpp @@ -108,7 +108,7 @@ static int32 ClickChangeDateCheat(int32 p1, int32 p2) if (p1 == _cur_year) return _cur_year; Date new_date = ConvertYMDToDate(p1, ymd.month, ymd.day); - LinkGraphSchedule::Instance()->ShiftDates(new_date - _date); + LinkGraphSchedule::instance.ShiftDates(new_date - _date); SetDate(new_date, _date_fract); EnginesMonthlyLoop(); SetWindowDirty(WC_STATUS_BAR, 0); diff --git a/src/linkgraph/linkgraphjob.cpp b/src/linkgraph/linkgraphjob.cpp index 30d68e4d8..20cbf3f64 100644 --- a/src/linkgraph/linkgraphjob.cpp +++ b/src/linkgraph/linkgraphjob.cpp @@ -21,6 +21,13 @@ LinkGraphJobPool _link_graph_job_pool("LinkGraphJob"); INSTANTIATE_POOL_METHODS(LinkGraphJob) +/** + * Static instance of an invalid path. + * Note: This instance is created on task start. + * Lazy creation on first usage results in a data race between the CDist threads. + */ +/* static */ Path *Path::invalid_path = new Path(INVALID_NODE, true); + /** * Create a link graph job from a link graph. The link graph will be copied so * that the calculations don't interfer with the normal operations on the diff --git a/src/linkgraph/linkgraphjob.h b/src/linkgraph/linkgraphjob.h index 00b65f208..b4587a784 100644 --- a/src/linkgraph/linkgraphjob.h +++ b/src/linkgraph/linkgraphjob.h @@ -343,6 +343,8 @@ public: */ class Path { public: + static Path *invalid_path; + Path(NodeID n, bool source = false); /** Get the node this leg passes. */ diff --git a/src/linkgraph/linkgraphschedule.cpp b/src/linkgraph/linkgraphschedule.cpp index 30ad20d4d..a65783a5e 100644 --- a/src/linkgraph/linkgraphschedule.cpp +++ b/src/linkgraph/linkgraphschedule.cpp @@ -18,6 +18,13 @@ #include "../safeguards.h" +/** + * Static instance of LinkGraphSchedule. + * Note: This instance is created on task start. + * Lazy creation on first usage results in a data race between the CDist threads. + */ +/* static */ LinkGraphSchedule LinkGraphSchedule::instance; + /** * Start the next job in the schedule. */ @@ -68,9 +75,8 @@ void LinkGraphSchedule::JoinNext() /* static */ void LinkGraphSchedule::Run(void *j) { LinkGraphJob *job = (LinkGraphJob *)j; - LinkGraphSchedule *schedule = LinkGraphSchedule::Instance(); - for (uint i = 0; i < lengthof(schedule->handlers); ++i) { - schedule->handlers[i]->Run(*job); + for (uint i = 0; i < lengthof(instance.handlers); ++i) { + instance.handlers[i]->Run(*job); } } @@ -90,12 +96,11 @@ void LinkGraphSchedule::SpawnAll() */ /* static */ void LinkGraphSchedule::Clear() { - LinkGraphSchedule *inst = LinkGraphSchedule::Instance(); - for (JobList::iterator i(inst->running.begin()); i != inst->running.end(); ++i) { + for (JobList::iterator i(instance.running.begin()); i != instance.running.end(); ++i) { (*i)->JoinThread(); } - inst->running.clear(); - inst->schedule.clear(); + instance.running.clear(); + instance.schedule.clear(); } /** @@ -135,15 +140,6 @@ LinkGraphSchedule::~LinkGraphSchedule() } } -/** - * Retrieve the link graph schedule or create it if necessary. - */ -/* static */ LinkGraphSchedule *LinkGraphSchedule::Instance() -{ - static LinkGraphSchedule inst; - return &inst; -} - /** * Spawn or join a link graph job or compress a link graph if any link graph is * due to do so. @@ -153,9 +149,9 @@ void OnTick_LinkGraph() if (_date_fract != LinkGraphSchedule::SPAWN_JOIN_TICK) return; Date offset = _date % _settings_game.linkgraph.recalc_interval; if (offset == 0) { - LinkGraphSchedule::Instance()->SpawnNext(); + LinkGraphSchedule::instance.SpawnNext(); } else if (offset == _settings_game.linkgraph.recalc_interval / 2) { - LinkGraphSchedule::Instance()->JoinNext(); + LinkGraphSchedule::instance.JoinNext(); } } diff --git a/src/linkgraph/linkgraphschedule.h b/src/linkgraph/linkgraphschedule.h index 5d1871962..ec22be316 100644 --- a/src/linkgraph/linkgraphschedule.h +++ b/src/linkgraph/linkgraphschedule.h @@ -51,8 +51,8 @@ protected: public: /* This is a tick where not much else is happening, so a small lag might go unnoticed. */ static const uint SPAWN_JOIN_TICK = 21; ///< Tick when jobs are spawned or joined every day. + static LinkGraphSchedule instance; - static LinkGraphSchedule *Instance(); static void Run(void *j); static void Clear(); diff --git a/src/linkgraph/mcf.cpp b/src/linkgraph/mcf.cpp index bd3794a63..6fed7adea 100644 --- a/src/linkgraph/mcf.cpp +++ b/src/linkgraph/mcf.cpp @@ -148,15 +148,14 @@ public: */ void SetNode(NodeID source, NodeID node) { - static const FlowStat::SharesMap empty; const FlowStatMap &flows = this->job[node].Flows(); FlowStatMap::const_iterator it = flows.find(this->job[source].Station()); if (it != flows.end()) { this->it = it->second.GetShares()->begin(); this->end = it->second.GetShares()->end(); } else { - this->it = empty.begin(); - this->end = empty.end(); + this->it = FlowStat::empty_sharesmap.begin(); + this->end = FlowStat::empty_sharesmap.end(); } } @@ -379,11 +378,10 @@ void MCF1stPass::EliminateCycle(PathVector &path, Path *cycle_begin, uint flow) */ bool MCF1stPass::EliminateCycles(PathVector &path, NodeID origin_id, NodeID next_id) { - static Path *invalid_path = new Path(INVALID_NODE, true); Path *at_next_pos = path[next_id]; /* this node has already been searched */ - if (at_next_pos == invalid_path) return false; + if (at_next_pos == Path::invalid_path) return false; if (at_next_pos == NULL) { /* Summarize paths; add up the paths with the same source and next hop @@ -431,7 +429,7 @@ bool MCF1stPass::EliminateCycles(PathVector &path, NodeID origin_id, NodeID next * could be found in this branch, thus it has to be searched again next * time we spot it. */ - path[next_id] = found ? NULL : invalid_path; + path[next_id] = found ? NULL : Path::invalid_path; return found; } diff --git a/src/saveload/linkgraph_sl.cpp b/src/saveload/linkgraph_sl.cpp index 20a3f4498..a65f4fc8a 100644 --- a/src/saveload/linkgraph_sl.cpp +++ b/src/saveload/linkgraph_sl.cpp @@ -220,7 +220,7 @@ static void Load_LGRJ() */ static void Load_LGRS() { - SlObject(LinkGraphSchedule::Instance(), GetLinkGraphScheduleDesc()); + SlObject(&LinkGraphSchedule::instance, GetLinkGraphScheduleDesc()); } /** @@ -246,7 +246,7 @@ void AfterLoadLinkGraphs() } } - LinkGraphSchedule::Instance()->SpawnAll(); + LinkGraphSchedule::instance.SpawnAll(); } /** @@ -278,7 +278,7 @@ static void Save_LGRJ() */ static void Save_LGRS() { - SlObject(LinkGraphSchedule::Instance(), GetLinkGraphScheduleDesc()); + SlObject(&LinkGraphSchedule::instance, GetLinkGraphScheduleDesc()); } /** @@ -286,7 +286,7 @@ static void Save_LGRS() */ static void Ptrs_LGRS() { - SlObject(LinkGraphSchedule::Instance(), GetLinkGraphScheduleDesc()); + SlObject(&LinkGraphSchedule::instance, GetLinkGraphScheduleDesc()); } extern const ChunkHandler _linkgraph_chunk_handlers[] = { diff --git a/src/station.cpp b/src/station.cpp index e0a48ef22..456262dea 100644 --- a/src/station.cpp +++ b/src/station.cpp @@ -106,7 +106,7 @@ Station::~Station() } lg->RemoveNode(this->goods[c].node); if (lg->Size() == 0) { - LinkGraphSchedule::Instance()->Unqueue(lg); + LinkGraphSchedule::instance.Unqueue(lg); delete lg; } } diff --git a/src/station_base.h b/src/station_base.h index b832a3c19..af4d206ba 100644 --- a/src/station_base.h +++ b/src/station_base.h @@ -37,6 +37,8 @@ class FlowStat { public: typedef std::map SharesMap; + static const SharesMap empty_sharesmap; + /** * Invalid constructor. This can't be called as a FlowStat must not be * empty. However, the constructor must be defined and reachable for diff --git a/src/station_cmd.cpp b/src/station_cmd.cpp index 0d8236ddc..37b553f1a 100644 --- a/src/station_cmd.cpp +++ b/src/station_cmd.cpp @@ -58,6 +58,13 @@ #include "safeguards.h" +/** + * Static instance of FlowStat::SharesMap. + * Note: This instance is created on task start. + * Lazy creation on first usage results in a data race between the CDist threads. + */ +/* static */ const FlowStat::SharesMap FlowStat::empty_sharesmap; + /** * Check whether the given tile is a hangar. * @param t the tile to of whether it is a hangar. @@ -3536,7 +3543,7 @@ void IncreaseStats(Station *st, CargoID cargo, StationID next_station_id, uint c if (ge2.link_graph == INVALID_LINK_GRAPH) { if (LinkGraph::CanAllocateItem()) { lg = new LinkGraph(cargo); - LinkGraphSchedule::Instance()->Queue(lg); + LinkGraphSchedule::instance.Queue(lg); ge2.link_graph = lg->index; ge2.node = lg->AddNode(st2); } else { @@ -3558,11 +3565,11 @@ void IncreaseStats(Station *st, CargoID cargo, StationID next_station_id, uint c if (ge1.link_graph != ge2.link_graph) { LinkGraph *lg2 = LinkGraph::Get(ge2.link_graph); if (lg->Size() < lg2->Size()) { - LinkGraphSchedule::Instance()->Unqueue(lg); + LinkGraphSchedule::instance.Unqueue(lg); lg2->Merge(lg); // Updates GoodsEntries of lg lg = lg2; } else { - LinkGraphSchedule::Instance()->Unqueue(lg2); + LinkGraphSchedule::instance.Unqueue(lg2); lg->Merge(lg2); // Updates GoodsEntries of lg2 } } @@ -3684,7 +3691,7 @@ static uint UpdateStationWaiting(Station *st, CargoID type, uint amount, SourceT if (ge.link_graph == INVALID_LINK_GRAPH) { if (LinkGraph::CanAllocateItem()) { lg = new LinkGraph(type); - LinkGraphSchedule::Instance()->Queue(lg); + LinkGraphSchedule::instance.Queue(lg); ge.link_graph = lg->index; ge.node = lg->AddNode(st); } else { diff --git a/src/toolbar_gui.cpp b/src/toolbar_gui.cpp index 00d844520..45d751dc5 100644 --- a/src/toolbar_gui.cpp +++ b/src/toolbar_gui.cpp @@ -1125,7 +1125,7 @@ void SetStartingYear(Year year) _settings_game.game_creation.starting_year = Clamp(year, MIN_YEAR, MAX_YEAR); Date new_date = ConvertYMDToDate(_settings_game.game_creation.starting_year, 0, 1); /* If you open a savegame as scenario there may already be link graphs.*/ - LinkGraphSchedule::Instance()->ShiftDates(new_date - _date); + LinkGraphSchedule::instance.ShiftDates(new_date - _date); SetDate(new_date, 0); } -- cgit v1.2.3-54-g00ecf