> On May 27, 2014, 9:47 a.m., Dominic Hamon wrote: > > src/jvm/jvm.cpp, line 23 > > <https://reviews.apache.org/r/21598/diff/1/?file=583937#file583937line23> > > > > please don't introduce static non-POD objects. See > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Global_Variables > > for details and potential workarounds. > > Timothy St. Clair wrote: > So... According to *this rule, the code is in violation all over the > place. > > I notice it most heavily in the java objects. I can change it, but even > the Jvm object is in violation. > ref1 - "have no dynamic initialization or destruction." > ref2 - "Likewise, global and static variables are destroyed when the > program terminates, regardless of whether the termination is by returning > from main() or by calling exit()" > > What I would prefer to do is: > static memory::shared_ptr<DynamicLibrary> libJvm; > static memory::shared_ptr<Jvm> instance; > > & actually have proper cleanup, b/c the Jvm object leaks. > > > > > Dominic Hamon wrote: > It is - see MESOS-1023. > > For now we can avoid adding new ones in this review by simply: > > static DynamicLibrary* libJvm() > { > static DynamicLibrary* libJvm = new DynamicLibrary(); > return libJvm; > } > > Note that memory::shared_ptr has an equivalent issue as it is non-POD > (has logic in the destructor). > > This is not just a style guide issue: We had a real bug in the past due > to destruction order of non-PODs. > > Timothy St. Clair wrote: > I can do this, but then we'll leak both the Jvm and libJvm on shutdown. > This just seems wrong, as the whole reason was to address leaks on > shutdown. > > I'm fully aware of the convention, but: > - I'm not a fan if it inhibits proper behavior. > - And again we violate it in other places. >
It's true that we'll not deallocate this memory on shutdown. It's debatable if that's actually a problem given that we are shutting down and the memory will be reclaimed anyway. The challenge here is that a static object that is non-POD causes real problems in the asynchronous environment in which we work. Not freeing memory allocated statically (would you prefer to use pthread_once instead of the static-in-a-function approach?) doesn't cause any problems. Violating the guideline in other places is not a good reason to add another violation. Instead, we should be striving to add no more violations and clean up the old ones. While thinking about this I reread the code - is there a reason why libJvm isn't just local to Jvm::create? It doesn't appear to be used outside of that method, unless I missed something horribly obvious. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21598/#review43981 ----------------------------------------------------------- On May 16, 2014, 7:02 p.m., Timothy St. Clair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21598/ > ----------------------------------------------------------- > > (Updated May 16, 2014, 7:02 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-1354 > https://issues.apache.org/jira/browse/MESOS-1354 > > > Repository: mesos-git > > > Description > ------- > > Fix for the coverity leak and update the code to leverage > stout/dynamiclibrary. > > > Diffs > ----- > > src/jvm/jvm.cpp 745b48f > > Diff: https://reviews.apache.org/r/21598/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy St. Clair > >
