You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2018/08/24 11:51:19 UTC
Re: Review Request 68484: Added macros to unconditionally disable
boost debug mode.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/
-----------------------------------------------------------
(Updated Aug. 24, 2018, 11:51 a.m.)
Review request for mesos and Alexander Rukletsov.
Changes
-------
Expanded description.
Summary (updated)
-----------------
Added macros to unconditionally disable boost debug mode.
Bugs: MESOS-9177
https://issues.apache.org/jira/browse/MESOS-9177
Repository: mesos
Description (updated)
-------
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,
we have to unconditionally disable Boost's debug mode to prevent
possible segfaults.
Diffs (updated)
-----
src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
Diff: https://reviews.apache.org/r/68484/diff/2/
Changes: https://reviews.apache.org/r/68484/diff/1-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
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 4:41 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.hpp
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/68484/diff/2/?file=2077072#file2077072line27>
> >
> > Let's just copy the comments here so the reader doesn't need to refer to another file and also leave a TODO.
It sounds like a good idea, but both comments are unified anyways later on in the chain so I think it's fine to leave it as is here.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
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
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 4:41 p.m., Chun-Hung Hsiao wrote:
> > src/master/master.hpp
> > Lines 32 (patched)
> > <https://reviews.apache.org/r/68484/diff/2/?file=2077071#file2077071line32>
> >
> > Let's leave a TODO here:
> > ```
> > // TODO(bevers): Remove this once we bump the minimum Boost version.
> > ```
Even for later boost versions we need to keep this definition to ensure the build fails due to conflicting definitions when a user tries to manually set the macro because he wants to use boost's debug features.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
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
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/68484/#comment291506>
Let's leave a TODO here:
```
// TODO(bevers): Remove this once we bump the minimum Boost version.
```
src/slave/slave.hpp
Lines 27 (patched)
<https://reviews.apache.org/r/68484/#comment291507>
Let's just copy the comments here so the reader doesn't need to refer to another file and also leave a TODO.
- Chun-Hung Hsiao
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
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207882
-----------------------------------------------------------
PASS: Mesos patch 68484 was successfully built and tested.
Reviews applied: `['68484']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2234/mesos-review-68484
- Mesos Reviewbot Windows
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
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
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.
Summary (updated)
-----------------
Unconditionally disabled boost debug mode.
Bugs: MESOS-9177
https://issues.apache.org/jira/browse/MESOS-9177
Repository: mesos
Description (updated)
-------
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