Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 27, 2016, 1:24 a.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Fixed IO tests.


Bugs: MESOS-3271 and MESOS-4711
https://issues.apache.org/jira/browse/MESOS-3271
https://issues.apache.org/jira/browse/MESOS-4711


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

Diff: https://reviews.apache.org/r/43799/diff/


Testing (updated)
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
./3rdpaty/libprocess/libprocess-tests --gtest_repeat=200 
--gtest_break_on_failure
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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



This breaks the IO based tests in Libprocess.

- Joris Van Remoortere


On Feb. 26, 2016, 9:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 26, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On Feb. 26, 2016, 9:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 26, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 26, 2016, 10:38 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Fixed comment formatting.


Bugs: MESOS-3271 and MESOS-4711
https://issues.apache.org/jira/browse/MESOS-3271
https://issues.apache.org/jira/browse/MESOS-4711


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

Diff: https://reviews.apache.org/r/43799/diff/


Testing
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 26, 2016, 10:29 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Changes
---

Added comments.


Bugs: MESOS-3271 and MESOS-4711
https://issues.apache.org/jira/browse/MESOS-3271
https://issues.apache.org/jira/browse/MESOS-4711


Repository: mesos


Description
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

Diff: https://reviews.apache.org/r/43799/diff/


Testing
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Joris Van Remoortere]

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




3rdparty/libprocess/src/libevent_poll.cpp (line 50)


Please add a very clear comment here that calls out the effect of this 
destructor invoking the event_free, which implies `event_del` which means the 
event will no longer be `pending`, and that's why we won't execute a 2nd time



3rdparty/libprocess/src/libevent_poll.cpp (lines 61 - 63)


can you add a comment explaining why we are testing `event_pending`?
Another comment for explaining that `event_active` triggers pollCallback to 
be run in the `event_loop`



3rdparty/libprocess/src/libevent_poll.cpp (lines 83 - 85)


let's call out here that we're binding in the destructor, and the 
consequences of that.



3rdparty/libprocess/src/libevent_poll.cpp (lines 90 - 91)


Please comment why we're using a weak_ptr here.



3rdparty/libprocess/src/libevent_poll.cpp (lines 91 - 93)


Please comment that this is the explicit order we want these 2 statements 
in.


- Joris Van Remoortere


On Feb. 24, 2016, 9:30 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 24, 2016, 9:30 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43799]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 24, 2016, 9:30 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 24, 2016, 9:30 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Alexander Rojas]

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

(Updated Feb. 24, 2016, 10:30 p.m.)


Review request for mesos, Joris Van Remoortere and Michael Park.


Summary (updated)
-

Removed race condition from libevent based poll implementation.


Bugs: MESOS-3271 and MESOS-4711
https://issues.apache.org/jira/browse/MESOS-3271
https://issues.apache.org/jira/browse/MESOS-4711


Repository: mesos


Description (updated)
---

Under certains circumstances, the future returned by poll is discarded right
after the event is triggered, this causes the event callback to be called
before the discard callback which results in an abort signal being raised
by the libevent library.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_poll.cpp 
461624ca003e97be5ea9cf956d86fc294e6f1bc1 

Diff: https://reviews.apache.org/r/43799/diff/


Testing
---

```bash
# On CentOS 6.7 running in virtualbox
../configure --enable-ssl --enable-libevent
make -j4 check
sudo ./bin/mesos-tests.sh 
--gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
--gtest_repeat=1000
```


Thanks,

Alexander Rojas

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Bernd Mathiske]

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_poll.cpp (line 56)


According to the style guide, we are usually using trailing underscores 
here.



3rdparty/libprocess/src/libevent_poll.cpp (line 57)


Although this should work as is, it is more straight forward and the common 
(generally safer) pattern to access the shared pointer here, not the weak one. 

  _ev.get() 
  
  (respectively ev_.get(), see above)


- Bernd Mathiske


On Feb. 19, 2016, 5:09 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 19, 2016, 5:09 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 43799: Removed race condition from libevent based poll implementation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43799]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 20, 2016, 1:09 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43799/
> ---
> 
> (Updated Feb. 20, 2016, 1:09 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Michael Park.
> 
> 
> Bugs: MESOS-3271 and MESOS-4711
> https://issues.apache.org/jira/browse/MESOS-3271
> https://issues.apache.org/jira/browse/MESOS-4711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under certains circumstances, the future returned by poll is discarded right
> after the event is triggered, this causes the event callback to be called
> before the discard callback which results in an abort signal being raised
> by the libevent library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 461624ca003e97be5ea9cf956d86fc294e6f1bc1 
> 
> Diff: https://reviews.apache.org/r/43799/diff/
> 
> 
> Testing
> ---
> 
> ```bash
> # On CentOS 6.7 running in virtualbox
> ../configure --enable-ssl --enable-libevent
> make -j4 check
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery" 
> --gtest_repeat=1000
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>