Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#930 closed defect (fixed)

cyclic references in Scheduler::Job

Reported by: Peter Owned by: Peter
Priority: critical Milestone: yat 0.17
Component: utility Version: 0.16.2
Keywords: Cc:


class Job declares the following:

// set of jobs that have to finish before this can run
std::set<boost::shared_ptr<Job> > prerequisite_;
// jobs that have *this as prerequisite
std::vector<boost::shared_ptr<Job> > observers_;

Normally this is not a problem because the set is becoming empty as jobs are completed, but if this doesn't happen, for example, because some jobs are not submitted (directly or indirectly) or the jobs are not completed because an exception is thrown, then we have a problem. Potentially (and quite like, tbh) two jobs will point to each other and creating a grid lock and the reference counters will never reach zero and the destructors are never called, which means in practice a memory leak.

Change History (3)

comment:1 Changed 3 years ago by Peter

Owner: changed from Jari Häkkinen to Peter
Status: newaccepted

The dependency structure could be stored in independent std::map with the downside that lookup is logN compared with constant.

The other alternative is to replace shared_ptr with Job* in either prerequisite_ or observers_. Two things are needed for this to work: 1) we don't need the signature of shared_ptr to, for example, pass to queues or other functions.

JobHandler::prepare(job) is recursive in that it calls prepare on all jobs in prerequisite_, which means that container needs access to the shared_ptr.

Similarly, JobHandler::post_process(job) loops over observers_ and if job was the last Job observers_[i] waited for, observers_[i] is passed to send2queue and thus observers_[i] cannot be a Job*.

Question is whether both containers should be moved to maps outside Job or only one and if so which one?

comment:2 Changed 3 years ago by Peter

Resolution: fixed
Status: acceptedclosed

In 3853:

fixes #930. Remove obervers_ variable in Job and instead keep that information in a map in private class Dependency.

comment:3 Changed 2 years ago by Peter

In 3921:

Remove comment that implementation might be problematic with reference to #930, since #930 has been closed and I can't see how there is a potential memory leak. refs #930.

Note: See TracTickets for help on using tickets.