You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/12/04 04:33:44 UTC
Re: Review Request 40501: Cleanup a leaked reference to a test process
living in the stack.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review108937
-----------------------------------------------------------
src/tests/fetcher_tests.cpp (lines 285 - 289)
<https://reviews.apache.org/r/40501/#comment168391>
This makes spawning and termination asymmetric! :(
Please follow the approach done here:
https://github.com/apache/mesos/blob/0.26.0-rc3/3rdparty/libprocess/src/tests/http_tests.cpp#L64-L117
- Ben Mahler
On Nov. 19, 2015, 9:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Two of the fetcher tests will spawn a process which is stored in the stack (i.e. local variable in the test). `spawn` will store a pointer to the process in libprocess's `ProcessManager`. When the test finishes, the process goes out of scope and is therefore lost. However, the process is **not** terminated.
>
> Failing to terminate this process will lead to an infinite loop in `~ProcessManager`, which is called in `process::finalize`. In `ProcessManager` 's destructor, we will loop and try to kill all processes. The process spawned in the test will be running. However, since the pointer lives in the stack, the `ProcessManager` will be unable to find the process and will thereby be stuck trying to kill a process it cannot find.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8
>
> Diff: https://reviews.apache.org/r/40501/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing:
>
> Insert a `process::finalize` in `src/test/main.cpp`. i.e.
> ```
> // Replace `return RUN_ALL_TESTS();` with this:
> int ret = RUN_ALL_TESTS();
> process::finalize();
> return ret;
> ```
>
> Then `make check GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40501: Cleanup a leaked reference to a test process
living in the stack.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Dec. 3, 2015, 7:33 p.m., Ben Mahler wrote:
> > src/tests/fetcher_tests.cpp, lines 285-289
> > <https://reviews.apache.org/r/40501/diff/1/?file=1133067#file1133067line285>
> >
> > This makes spawning and termination asymmetric! :(
> >
> > Please follow the approach done here:
> >
> > https://github.com/apache/mesos/blob/0.26.0-rc3/3rdparty/libprocess/src/tests/http_tests.cpp#L64-L117
Makes sense, this will be better.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review108937
-----------------------------------------------------------
On Nov. 19, 2015, 1:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2015, 1:51 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Two of the fetcher tests will spawn a process which is stored in the stack (i.e. local variable in the test). `spawn` will store a pointer to the process in libprocess's `ProcessManager`. When the test finishes, the process goes out of scope and is therefore lost. However, the process is **not** terminated.
>
> Failing to terminate this process will lead to an infinite loop in `~ProcessManager`, which is called in `process::finalize`. In `ProcessManager` 's destructor, we will loop and try to kill all processes. The process spawned in the test will be running. However, since the pointer lives in the stack, the `ProcessManager` will be unable to find the process and will thereby be stuck trying to kill a process it cannot find.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8
>
> Diff: https://reviews.apache.org/r/40501/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing:
>
> Insert a `process::finalize` in `src/test/main.cpp`. i.e.
> ```
> // Replace `return RUN_ALL_TESTS();` with this:
> int ret = RUN_ALL_TESTS();
> process::finalize();
> return ret;
> ```
>
> Then `make check GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40501: Cleanup a leaked reference to a test process
living in the stack.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 3, 2015, 7:33 p.m., Ben Mahler wrote:
> > src/tests/fetcher_tests.cpp, lines 285-289
> > <https://reviews.apache.org/r/40501/diff/1/?file=1133067#file1133067line285>
> >
> > This makes spawning and termination asymmetric! :(
> >
> > Please follow the approach done here:
> >
> > https://github.com/apache/mesos/blob/0.26.0-rc3/3rdparty/libprocess/src/tests/http_tests.cpp#L64-L117
>
> Bernd Mathiske wrote:
> Makes sense, this will be better.
I'll open a separate review to fix this, because this commit wasn't reverted.
See: https://reviews.apache.org/r/40978/
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review108937
-----------------------------------------------------------
On Nov. 19, 2015, 1:51 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2015, 1:51 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Two of the fetcher tests will spawn a process which is stored in the stack (i.e. local variable in the test). `spawn` will store a pointer to the process in libprocess's `ProcessManager`. When the test finishes, the process goes out of scope and is therefore lost. However, the process is **not** terminated.
>
> Failing to terminate this process will lead to an infinite loop in `~ProcessManager`, which is called in `process::finalize`. In `ProcessManager` 's destructor, we will loop and try to kill all processes. The process spawned in the test will be running. However, since the pointer lives in the stack, the `ProcessManager` will be unable to find the process and will thereby be stuck trying to kill a process it cannot find.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8
>
> Diff: https://reviews.apache.org/r/40501/diff/
>
>
> Testing
> -------
>
> `make check`
>
> Additional testing:
>
> Insert a `process::finalize` in `src/test/main.cpp`. i.e.
> ```
> // Replace `return RUN_ALL_TESTS();` with this:
> int ret = RUN_ALL_TESTS();
> process::finalize();
> return ret;
> ```
>
> Then `make check GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
>
>
> Thanks,
>
> Joseph Wu
>
>