You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/11/19 22:51:34 UTC
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/
-----------------------------------------------------------
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107301
-----------------------------------------------------------
Ship it!
Great catach!
- Bernd Mathiske
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 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
>
>
Re: Review Request 40501: Cleanup a leaked reference to a test process
living in the stack.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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 Nov. 19, 2015, 6:16 p.m., Neil Conway wrote:
> > Good find. I wonder:
> >
> > (a) is there some general advice we should give to people implementing Processes (e.g., "always provide a destructor that does terminate/wait" -- that is probably too broad though). Would be nice to add this to the libprocess README.
> > (b) does this problem occur anywhere else? and/or is there a way to detect it?
[~neilc] a) I agree that we should provide documentation in this regard. This kind of pattern is too easily overlooked, also by reviewers, as exemplified when the bug got introduced: https://reviews.apache.org/r/27483/
b) For test code (such as is the case here), we could put something into the inherited fixture that scans for orphaned processes at TearDown.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107290
-----------------------------------------------------------
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 Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 20, 2015, 2:16 a.m., Neil Conway wrote:
> > Good find. I wonder:
> >
> > (a) is there some general advice we should give to people implementing Processes (e.g., "always provide a destructor that does terminate/wait" -- that is probably too broad though). Would be nice to add this to the libprocess README.
> > (b) does this problem occur anywhere else? and/or is there a way to detect it?
>
> Bernd Mathiske wrote:
> [~neilc] a) I agree that we should provide documentation in this regard. This kind of pattern is too easily overlooked, also by reviewers, as exemplified when the bug got introduced: https://reviews.apache.org/r/27483/
> b) For test code (such as is the case here), we could put something into the inherited fixture that scans for orphaned processes at TearDown.
Note that there is some assymmetry in the API: when *not giving* a `manage` arg to `spawn`. you get the default value `false`; but you then *do need to add code* to terminate and wait for the process.
I personally would naively have expected an API to either (i) require me to be explicitly in both places (explictly set `manage=false`, and to manual `terminate` and `wait`), or (ii) it just do The Right Thing if I was brief (I called `spawn` with default args, and do not have to worry about cleanup).
I.e., wouldn't it in the long run make more sense the change `spawn` to `PID<T> spawn(T& t, bool manage = true)` than to add more documentation?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107290
-----------------------------------------------------------
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 Joseph Wu <jo...@mesosphere.io>.
> On Nov. 19, 2015, 6:16 p.m., Neil Conway wrote:
> > Good find. I wonder:
> >
> > (a) is there some general advice we should give to people implementing Processes (e.g., "always provide a destructor that does terminate/wait" -- that is probably too broad though). Would be nice to add this to the libprocess README.
> > (b) does this problem occur anywhere else? and/or is there a way to detect it?
>
> Bernd Mathiske wrote:
> [~neilc] a) I agree that we should provide documentation in this regard. This kind of pattern is too easily overlooked, also by reviewers, as exemplified when the bug got introduced: https://reviews.apache.org/r/27483/
> b) For test code (such as is the case here), we could put something into the inherited fixture that scans for orphaned processes at TearDown.
>
> Benjamin Bannier wrote:
> Note that there is some assymmetry in the API: when *not giving* a `manage` arg to `spawn`. you get the default value `false`; but you then *do need to add code* to terminate and wait for the process.
>
> I personally would naively have expected an API to either (i) require me to be explicitly in both places (explictly set `manage=false`, and to manual `terminate` and `wait`), or (ii) it just do The Right Thing if I was brief (I called `spawn` with default args, and do not have to worry about cleanup).
>
> I.e., wouldn't it in the long run make more sense the change `spawn` to `PID<T> spawn(T& t, bool manage = true)` than to add more documentation?
[~neilc]
a) I agree that documentation is a good direction. (I'll draft something.)
[~bernd-mesos]
b) We already have an orphaned process detector in `src/tests/environment.cpp` `Environment::TearDown`. However, this only catches **orphaned** processes (whose parents have been killed :( ). This HTTP process's parent is the test process.
There is an endpoint `/__processes__` which we might be able to use to check for extraneous processes. But this might be messy, since we have a great deal of global processes.
[~bbannier]
The `manage` argument will only change ownership of the process pointer. If you dig into the `GarbageCollector`, you'll see that all it does is `delete` the pointer when the process terminates. The original process is still in charge of its own termination.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107290
-----------------------------------------------------------
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 Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107290
-----------------------------------------------------------
Ship it!
Good find. I wonder:
(a) is there some general advice we should give to people implementing Processes (e.g., "always provide a destructor that does terminate/wait" -- that is probably too broad though). Would be nice to add this to the libprocess README.
(b) does this problem occur anywhere else? and/or is there a way to detect it?
- Neil Conway
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 Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40501/#review107415
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40501]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>