You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/08/27 02:15:00 UTC
Review Request 37821: Join threads in libprocess when shutting down.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158
Repository: mesos
Description
-------
Join threads in libprocess when shutting down.
Diffs
-----
3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
Diff: https://reviews.apache.org/r/37821/diff/
Testing
-------
After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
make check
Thanks,
Greg Mann
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96623
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37821]
All tests passed.
- Mesos ReviewBot
On Aug. 27, 2015, 12:14 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 27, 2015, 2:12 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2213
> > <https://reviews.apache.org/r/37821/diff/1/?file=1055467#file1055467line2213>
> >
> > This leaks "request".
Thanks Neil! Comments addressed.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96625
-----------------------------------------------------------
On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 4:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96625
-----------------------------------------------------------
3rdparty/libprocess/src/event_loop.hpp (line 45)
<https://reviews.apache.org/r/37821/#comment152218>
Might be good to document that this method is async (i.e., we don't guarantee that the event loop is stopped when the function returns).
3rdparty/libprocess/src/libev.cpp (line 36)
<https://reviews.apache.org/r/37821/#comment152216>
The comment above says this code block "defines the initial values for all of the declarations made in libev.hpp"; unfortunately this doesn't apply to shutdown_watcher.
3rdparty/libprocess/src/process.cpp (line 2211)
<https://reviews.apache.org/r/37821/#comment152219>
This leaks "request".
- Neil Conway
On Aug. 27, 2015, 12:14 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 12:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96654
-----------------------------------------------------------
Sorry folks, there seems to be some kind of data race here... after running:
"make check"
if I then run:
"for n in {1..300}; do echo $n; 3rdparty/libprocess/tests; done"
it will hang eventually, always right when the tests are exiting. I missed this previously because I was unaware that using the "--gtest_repeat" flag doesn't seem to trigger libprocess's process initialization/shutdown with each iteration. I'll have a look and try to figure out what's causing this.
- Greg Mann
On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 4:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96653
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37821]
All tests passed.
- Mesos ReviewBot
On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 4:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 27, 2015, 6:14 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2212
> > <https://reviews.apache.org/r/37821/diff/2/?file=1055749#file1055749line2212>
> >
> > Somewhat race-prone: we might see "shutting_down.load() == false", proceed to deliver the inbound message, and yet the shutdown code can proceed concurrently. After a bit of poking I couldn't find a situation in which that would be problematic, but maybe worth exploring if there's a known data race/hang...
Thanks Neil, good point. It turns out the race condition was occurring in schedule() and was easily fixed by moving a boolean test. However, you're right that currently it's possible for processes to get queued up in ProcessManager::handle() after shutting_down has been set to true, and this is not great.
I could move the "if (shutting_down.load())" test closer to the actual calls to deliver() and dispatch(), which would require duplicating it a number of times. It would be messy, but would lessen the raciness. Placing the test in deliver() seems like a lot of unnecessary work when internal libprocess messages are sent, and we still want to let internal processes send/receive messages while they're terminating.
Perhaps there's another superior location for this test that I'm not finding?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96655
-----------------------------------------------------------
On Aug. 27, 2015, 10:59 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96655
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 2210)
<https://reviews.apache.org/r/37821/#comment152273>
Somewhat race-prone: we might see "shutting_down.load() == false", proceed to deliver the inbound message, and yet the shutdown code can proceed concurrently. After a bit of poking I couldn't find a situation in which that would be problematic, but maybe worth exploring if there's a known data race/hang...
- Neil Conway
On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 4:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review97758
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37821]
All tests passed.
- Mesos ReviewBot
On Sept. 4, 2015, 2:23 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 2:23 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp ee7906470069b0391dde7cd685b1d4eb3a158c03
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/
-----------------------------------------------------------
(Updated Sept. 4, 2015, 2:23 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
Changes
-------
Rebase.
Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158
Repository: mesos
Description
-------
Join threads in libprocess when shutting down.
Diffs (updated)
-----
3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
3rdparty/libprocess/src/libevent.cpp ee7906470069b0391dde7cd685b1d4eb3a158c03
3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
Diff: https://reviews.apache.org/r/37821/diff/
Testing
-------
After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
make check
Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
Thanks,
Greg Mann
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96823
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37821]
All tests passed.
- Mesos ReviewBot
On Aug. 28, 2015, 1:34 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 1:34 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
Changes
-------
Addressed comments.
Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158
Repository: mesos
Description
-------
Join threads in libprocess when shutting down.
Diffs (updated)
-----
3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
Diff: https://reviews.apache.org/r/37821/diff/
Testing
-------
After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
make check
Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
Thanks,
Greg Mann
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Neil Conway <ne...@gmail.com>.
> On Aug. 27, 2015, 11:35 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2195
> > <https://reviews.apache.org/r/37821/diff/3/?file=1056787#file1056787line2195>
> >
> > Why "const long& cpus", rather than just "long cpus"?
> >
> > Anyway, I think it might be better to move the calculation of "cpus" (along with the explanatory comment) into init_threads().
>
> Greg Mann wrote:
> I originally left the cpu calculation outside of init_threads() because it's used later on in a log message, but I agree that it's a nicer to put it in there, so I let init_threads() return cpus, leaving it as const since it shouldn't be altered if it's going to continue reflecting the number of processing threads created.
I'm not sure "const" is buying you enough to merit including, personally -- up to you.
Otherwise, looks good to me!
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96790
-----------------------------------------------------------
On Aug. 28, 2015, 1:34 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 27, 2015, 11:35 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2195
> > <https://reviews.apache.org/r/37821/diff/3/?file=1056787#file1056787line2195>
> >
> > Why "const long& cpus", rather than just "long cpus"?
> >
> > Anyway, I think it might be better to move the calculation of "cpus" (along with the explanatory comment) into init_threads().
I originally left the cpu calculation outside of init_threads() because it's used later on in a log message, but I agree that it's a nicer to put it in there, so I let init_threads() return cpus, leaving it as const since it shouldn't be altered if it's going to continue reflecting the number of processing threads created.
> On Aug. 27, 2015, 11:35 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2205
> > <https://reviews.apache.org/r/37821/diff/3/?file=1056787#file1056787line2205>
> >
> > std::cref, assuming you can change the parameter to const ref above.
Thanks Neil! Comments addressed.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96790
-----------------------------------------------------------
On Aug. 28, 2015, 1:34 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96790
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 660)
<https://reviews.apache.org/r/37821/#comment152435>
AFAICS you can make this parameter a const ref.
3rdparty/libprocess/src/process.cpp (line 2181)
<https://reviews.apache.org/r/37821/#comment152433>
Why "const long& cpus", rather than just "long cpus"?
Anyway, I think it might be better to move the calculation of "cpus" (along with the explanatory comment) into init_threads().
3rdparty/libprocess/src/process.cpp (line 2191)
<https://reviews.apache.org/r/37821/#comment152436>
std::cref, assuming you can change the parameter to const ref above.
- Neil Conway
On Aug. 27, 2015, 10:59 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/#review96802
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37821]
All tests passed.
- Mesos ReviewBot
On Aug. 27, 2015, 10:59 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 10:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
>
>
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Join threads in libprocess when shutting down.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
> 3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
> 3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
> 3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
>
> Diff: https://reviews.apache.org/r/37821/diff/
>
>
> Testing
> -------
>
> After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
>
> make check
>
>
> Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
>
> for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 10:59 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
Changes
-------
Fixed race condition and moved thread initialization into ProcessManager.
Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158
Repository: mesos
Description
-------
Join threads in libprocess when shutting down.
Diffs (updated)
-----
3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
Diff: https://reviews.apache.org/r/37821/diff/
Testing (updated)
-------
After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
make check
Also, to check for race conditions related to the initialization/shutdown of libprocess, try something like:
for n in {1..1000}; do echo $n; 3rdparty/libprocess/tests --gtest_filter=ProcessTest.Spawn; done
Thanks,
Greg Mann
Re: Review Request 37821: Join threads in libprocess when shutting
down.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37821/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 4:28 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched to 'mcypark'.
Changes
-------
Addressed comments.
Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158
Repository: mesos
Description
-------
Join threads in libprocess when shutting down.
Diffs (updated)
-----
3rdparty/libprocess/src/event_loop.hpp 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30
3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840
3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5
3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6
Diff: https://reviews.apache.org/r/37821/diff/
Testing
-------
After configuring with both "../configure" and "../configure --enable-libevent --enable-ssl":
make check
Thanks,
Greg Mann