----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24535/#review56597 -----------------------------------------------------------
Looks good modulo some issues below, would like to take a final pass when you update the APR() abstraction to be thread safe. 3rdparty/libprocess/3rdparty/Makefile.am <https://reviews.apache.org/r/24535/#comment96964> You also need to update stout's two makefiles to add: svn.hpp svn_tests.cpp 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment96977> s/finalize/initialize/ 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment96983> These apr_lifetime calls are not thread safe, so I believe there is an issue here when `svn::diff()` and `svn::patch()` are both called for the first time concurrently from different threads. At that point, we make two concurrent calls to `apr_initialize()`, which is undefined. We could add locking here, at which point we could also have apr_terminate() safely inside the destructor. There is a thread about the thread safety (no pun intended), here: http://mail-archives.apache.org/mod_mbox/apr-dev/201309.mbox/%3ccane84rwsdaptdiprv6inawx2vdttfggyggaxssrh4xampca...@mail.gmail.com%3E 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment96976> Why setup an atexit handler as opposed to calling apr_terminate() directly within ~APR()? I ask because, presumably the point of this APR abstraction is to provide APR initialization and termination bounded to the scope of an APR() object. Anything I'm missing? 3rdparty/libprocess/3rdparty/stout/tests/svn_tests.cpp <https://reviews.apache.org/r/24535/#comment96997> Any chance you could add another test for empty diff / patch? - Ben Mahler On Oct. 12, 2014, 4:14 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24535/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2014, 4:14 a.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > Note that this hard codes the location of the subversion and Apache Portable > Runtime (APR) headers. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am > 256df0bb5557ebe6c75099d35c284804c9e57253 > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/svn_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/24535/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
