> On Jan. 2, 2018, 5:12 p.m., Zhitao Li wrote: > > src/slave/containerizer/mesos/provisioner/provisioner.cpp > > Line 568 (original), 568 (patched) > > <https://reviews.apache.org/r/64885/diff/1/?file=1929384#file1929384line568> > > > > I don't think the callback here uses any internal state of calling > > actor, so I don't see it really necessary that continuation *must* happen > > in `self()`. > > > > Do we have a rule to require even continuation without side effect to > > calling actor be defered?
This callback make use of the member variable `rootfs` which will be accessed via a `this` pointer; `containerId`, `imageInfo`, and `rootfs` are all captured by value. My _personal_ rule of thumb is to not use default capture, but instead list every captured variable explicitly in the capture list. I also like to prefix uses of member variables with `this` in the lambda body. I feel that makes it easier to spot such issues even without static analysis. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64885/#review194629 ----------------------------------------------------------- On Jan. 2, 2018, 11:38 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64885/ > ----------------------------------------------------------- > > (Updated Jan. 2, 2018, 11:38 a.m.) > > > Review request for mesos, Gilbert Song and Zhitao Li. > > > Repository: mesos > > > Description > ------- > > In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer > checkpointing to the provisioner, but did not make sure that > continutations making use of internal state of the provisioner actor > installed on futures where always executed in the actor's context. > This was problematic as continuations could be executed while actor > state was changing (data races), or after the the actor had terminated > (use after free). > > In this patch we instead defer execution of continutions to the actor > owning the data. > > This issue was identified with the clang-tidy `mesos-this-capture` > check. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/provisioner.cpp > 61e771872ba2c3f5ecabbe085db102433c7bac3f > > > Diff: https://reviews.apache.org/r/64885/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
