> On Oct. 25, 2018, 8:07 a.m., Qian Zhang wrote: > > src/linux/cgroups.cpp > > Lines 1071-1080 (original), 1071-1089 (patched) > > <https://reviews.apache.org/r/69123/diff/1/?file=2101988#file2101988line1071> > > > > I see we already have an onAny callback `_listen`, can we close the fd > > at the end of that method (i.e., when we are sure that we will not listen > > anymore)? And then we could change the code here like below: > > ``` > > if (reading.isSome()) { > > reading->discard(); > > } else if (eventfd.isSome()) { > > Try<Nothing> unregister = unregisterNotifier(eventfd.get()); > > if (unregister.isError()) { > > LOG(ERROR) << "Failed to unregister eventfd: " << > > unregister.error(); > > } > > } > > > > ``` > > Benjamin Mahler wrote: > I originally tried to simplify the lifecycle of the listener, but it was > implemented this way because some callers call listen multiple times. > > create listener > listen > listen > listen > terminate > > Qian Zhang wrote: > Yeah, I know we support that. So that's why I suggested to close the fd > (`unregisterNotifier(eventfd.get())`) at the end of `_listen`, and then even > the caller call `listen` again, we will actually do nothing but just return > `error` (since `error` has been set in `_listen`). I think if we are sure > that we will not listen again (i.e., the first time when `_listen` is call > with `error` set), we should close the fd immediately rather than closing it > until `finalize` is called. > > Benjamin Mahler wrote: > The code that was suggested will leak the fd: > > ``` > if (reading.isSome()) { > reading->discard(); > } else if (eventfd.isSome()) { > Try<Nothing> unregister = unregisterNotifier(eventfd.get()); > if (unregister.isError()) { > LOG(ERROR) << "Failed to unregister eventfd: " << unregister.error(); > } > } > ``` > > If we're in `finalize()` and `reading.isSome()`, this would never close > the fd since the dispatch to `_listen` will be dropped (the Process > terminated). > > Are you suggesting something other than this code? > > Qian Zhang wrote: > You are right, that will leak the fd, so I think we should go with your > solution. But I'd still suggest to close the fd at the end of `_listen` since > we are sure the listener will not listen again (i.e., the fd will not be used > by the listener anymore). How do you think?
If we do closes in both locations, we no longer have the fd lifecycle be controlled by the process lifetime (initialize to finalize). The reader now needs to understand that it's either (initialize to finalize) or (initialize to first error). We will also have to be careful to avoid double close from both `_listen` and `finalize` (e.g. clear the event fd option). Seems not worth the additional complexity. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69123/#review210024 ----------------------------------------------------------- On Oct. 26, 2018, 3:25 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69123/ > ----------------------------------------------------------- > > (Updated Oct. 26, 2018, 3:25 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-9334 > https://issues.apache.org/jira/browse/MESOS-9334 > > > Repository: mesos > > > Description > ------- > > The cgroups event notifier was closing the eventfd while an > `io::read()` operation may be in progress. This can lead to > bugs where the fd gets re-used and read from a stale io::read. > > > Diffs > ----- > > src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 > > > Diff: https://reviews.apache.org/r/69123/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
