> On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, lines 1204-1210 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1204> > > > > To be consistent and symetric, putting `registerNotifier` in > > `initialize` is better than putting it here. What if the caller creates the > > process but forgets to spawn it? Do you leak an opened event file? > > > > I would rather keep the existing constructor. We can hide the public > > constructor and make those exposed public functions friend. Validations can > > go into those public exposed functions. > > Chi Zhang wrote: > If we validate in the public / friended function by calling > registerNotifier and closing the fd, can we just assume it'd always succeed > in the constructor / initialize (which are void functions)? even if we could, > it's still 2 pieces of the same code. > > I agree with being consistent and symetric, and to address the above > leak, how about we put unregisterNotifier to destructor? On the construction > side, we use a public creation function as the only construction path that > provides explicit validation; the private constructor still takes the opened > eventfd to avoid some code duplication? Does it sound like a better balance? > > IMHO more commonly client code forgets about proper closing after it has > got the results than forgetting about getting the results -- i.e., it's > probably more likely to forget to 'terminate' than to 'spawn'. Terminate > requires an explict call; but compiler invokes destructor for us -- we could > reduce the possible leak more for client code this way? Just my 2 cents.
Putting `registerNotifier` in static create function and putting `unregiserNotifier` in destructor does not sound very symetric to me. The lifecycle of the eventfd is controlled by two entities: static create and event::Listener. I still sugguest putting `registerNotifier` and `unregisterNotifier` in `initialize` and `finalize`. You need to store an `Option<Error> error;` in `event::Listener`. If `error.isSome()`, simply fail all `listen` attempts. During initializtion of event::Listener, register the notifier and set `error` if any error occurs. > On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 1318 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1318> > > > > Why do you need a Owned<Promise> here? Can it simply be: > > ``` > > Option<Promise<uint64>> promsie; > > ``` > > Chi Zhang wrote: > I don't think that'd compile since promise is not copyable and not > assignable. More, Promise doesn't seem to have a 'reset' function to reset > its associated future to PENDING again (could this be a useful patch, BTW?), > which means a new promise has to be created to have a unset future. > > How about Result<Promise*> promise? We can have: > None -- whenever there isn't a pending read and no errors have > encounterred before. Also the initial value. > Some -- Have a pending read, so we can reuse this promise. When the read > completes successfully, future is set and promise is (deleted and) reset to > none again. > Error -- Set if a read is failed. The public listen functiion returns > error from this point on and won't start a new listen. Leave it to the client > code to clean this process up. > > Re: what if client discards the future returned by listen: > How about change it to discard the current read if it hasn't completed or > had an error? If it has, use the above logic. Basically future returned by > listen only controls a single listen; client uses the process pointer from > the constructor path to control the process' lifetime? IC. I think Owned has a `reset` function. Maybe use that? Regarding the discard semantics, what if there are two entities called `listen`? If one of them discard the future, what happens to the other one? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31008/#review72643 ----------------------------------------------------------- On Feb. 13, 2015, 6:07 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31008/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 6:07 p.m.) > > > Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. > > > Bugs: mesos-2136 > https://issues.apache.org/jira/browse/mesos-2136 > > > Repository: mesos > > > Description > ------- > > Refactored EventListener to allow for continuous monitoring of an event. > > > Diffs > ----- > > src/linux/cgroups.hpp bf8efee05b995a37d4e6e7679a493b2d5238aa0b > src/linux/cgroups.cpp b6f75b14dea7609b90627eccdd33ef891ed9d21f > > Diff: https://reviews.apache.org/r/31008/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
