> On Dec. 6, 2016, 10:42 p.m., Benjamin Mahler wrote: > > Thanks Ilya! > > > > Have you looked at other pidfile related libraries? Looks like BSD provides > > some functions for this (they're also available on Linux): > > https://www.freebsd.org/cgi/man.cgi?query=pidfile&sektion=3&manpath=FreeBSD+6.1-RELEASE > > https://linux.die.net/man/3/pidfile > > > > It would be great to provide an os-agnostic library within stout for this, > > e.g.: > > > > ``` > > pidfile::open() > > pidfile::close() > > pidfile::remove() > > ``` > > > > I haven't looked too deeply at what we should have these take and return, > > but I noticed the use of RAII seemed unnecessary in your patch (only the > > .cpp implementation uses the exposed RAII class? If so, perhaps we can just > > leave RAII out for now since the callers do not use it). Also it would be > > great to which process is already running (which is provided by the > > BSD/Linux functions). Also, another suggestion as a first step here is to > > avoid Windows support entirely for now if we don't implement the locking > > aspect of this (as that seems necessary for this to be safely usable?) > > > > We could structure the patches like this: > > > > (1) Addition of pidfile utilities to stout. > > (2) Tests of the pidfile utilities in stout (should be possible?) > > (3) Integration of the pidfile utilities into mesos (manually test since > > the logic will reside in the mains?) > > Ilya Pronin wrote: > Thanks for the feedback! > > > Looks like BSD provides some functions for this (they're also available > on Linux) > > Yes, I saw `libbsd` but since it's not part of POSIX I decided to make > things portable. > > > Also it would be great to which process is already running > > Good idea! I will add it. > > > I noticed the use of RAII seemed unnecessary in your patch (only the > .cpp implementation uses the exposed RAII class? If so, perhaps we can just > leave RAII out for now since the callers do not use it). > > My intention was to try to be nice and clean up the PID file when daemon > exits through `std::exit()` or terminates on `SIGTERM`. For handling the > first case I created a static RAII object so its destructor will be called > during `std::exit()` cleanup. We could place this object in `main()`. But > here comes the second case. Our daemons perform default `SIGTERM` action > which is `term`. There's a signal handler in `logging/logging.cpp` where we > can unlink the file. For that I introduced a helper function and placed the > RAII object inside `common/pid_file.cpp`. > > So I think a RAII class is a good thing for implementing such thing. I'm > OK with moving it to stout and restructuring patches. > > Also I just thought that I should check if the destructor is executing > inside the process that created the PID file to protect from unlinking by > forked processes. > > What do you think?
A problem with the RAII class here is that we sometimes manually have to release the lock (in case of a signal). When I look at this patch I read something like "Hey, you don't have to close & remove the pidfile explicitly, we'll do that for you... well, unless your process is signaled". I'm inclined to require users call `removePidFile()` explicitly to avoid creaing a false feeling of safety. Another question: Why do you want to expose `PIDFile` class in the `.hpp`? - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54081/#review158240 ----------------------------------------------------------- On Nov. 28, 2016, 12:16 p.m., Ilya Pronin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54081/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2016, 12:16 p.m.) > > > Review request for mesos, Benjamin Mahler and Vinod Kone. > > > Bugs: MESOS-1648 > https://issues.apache.org/jira/browse/MESOS-1648 > > > Repository: mesos > > > Description > ------- > > The PID file is created and kept locked while master / agent is running to > prevent other instances with the same PID file location from starting. > > > Diffs > ----- > > src/CMakeLists.txt d6e213686a44fbca0ed841594ce803151224a5a7 > src/Makefile.am dd1626d177b38a6613f18f32bb0668abbb5100e0 > src/common/pid_file.hpp PRE-CREATION > src/common/pid_file.cpp PRE-CREATION > src/logging/logging.cpp 70d66a5c396f709e8f27ad0d51315ed6d257f73b > src/master/main.cpp fa7ba1310142a3bef71379ba37fded9b8390aae9 > src/slave/main.cpp 8010f8e229e2d820649750c9db0456ecd1b854d3 > src/tests/common/pid_file_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54081/diff/ > > > Testing > ------- > > Added test to verify that PID file is created upon `PIDFile` object creation > and deleted upon its destruction. Ran `make check`. > > > Thanks, > > Ilya Pronin > >
