mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 68484: Unconditionally disabled boost debug mode.
Date Fri, 24 Aug 2018 15:40:42 GMT


> On Aug. 24, 2018, 10:44 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp
> > Lines 33-34 (patched)
> > <https://reviews.apache.org/r/68484/diff/1/?file=2076510#file2076510line33>
> >
> >     IMO this solution is not maintable and too low level in Mesos proper. I believe
we should instead do one of following:
> >     
> >     1. Provide a header and type in `stout` wrapping `boost/circular_buffer.hpp`
which performs this setup. That way Mesos does not have to worry about low-level details which
are nicely encapsulated in `stout`.
> >     2. Add these defines whenever we make use of Boost (simple in cmake, slightly
more messy in autotools). That way users cannot forget to add them when using `boost/circular_buffer.hpp`.
> >     
> >     I personally would strongly prefer 1 over 2.
> 
> Benno Evers wrote:
>     We certainly shouldn't do (2), I just recently had a review where I removed exactly
such code (https://reviews.apache.org/r/67632/) because it can lead to hard-to-understand
build errors when trying to build things outside the Mesos, i.e. building Frameworks.
>     
>     We might think about (1), but it's not as clear-cut as you suggest because Boost's
behaviour is not unconditionally a bug, it only becomes one due to the specific interaction
with the request batching code in master. If any of our public headers end up including this
new `stout/circular_buffer.hpp`, this again might create problems for users including this
header outside of the Mesos build system.
>     
>     Either way, I would suggest not doing any of this in *this* review, since there are
two many open design questions to be discussed (i.e. do we make `stout/boost.hpp` or one header
for every individual feature used, do we provide methods to configure the default options,
etc.), and we'd be holding up the 1.7 release while we argue over them.

After offline discussions, I created a follow-up review implementing (1).


- Benno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207865
-----------------------------------------------------------


On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2018, 12:28 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9177
>     https://issues.apache.org/jira/browse/MESOS-9177
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
> 
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
> 
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8 
>   src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6 
> 
> 
> Diff: https://reviews.apache.org/r/68484/diff/2/
> 
> 
> Testing
> -------
> 
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel
(pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message