You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/04/12 16:02:51 UTC
Review Request 46094: Fixed memory leaks in Encoder/Decoder tests in
libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/
-----------------------------------------------------------
Review request for mesos, Joris Van Remoortere and Joseph Wu.
Repository: mesos
Description
-------
Fixed memory leaks in Encoder/Decoder tests in libprocess.
Diffs
-----
3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
Diff: https://reviews.apache.org/r/46094/diff/
Testing
-------
make check
Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
Thanks,
Neil Conway
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/#review128863
-----------------------------------------------------------
Ship it!
- Benjamin Bannier
On April 13, 2016, 4:23 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46094/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 4:23 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed memory leaks in Encoder/Decoder tests in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
>
> Diff: https://reviews.apache.org/r/46094/diff/
>
>
> Testing
> -------
>
> make check
>
> Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests
in libprocess.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/
-----------------------------------------------------------
(Updated April 13, 2016, 2:23 p.m.)
Review request for mesos, Joris Van Remoortere and Joseph Wu.
Changes
-------
Use smart pointers.
Repository: mesos
Description
-------
Fixed memory leaks in Encoder/Decoder tests in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
Diff: https://reviews.apache.org/r/46094/diff/
Testing
-------
make check
Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
Thanks,
Neil Conway
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests
in libprocess.
Posted by Neil Conway <ne...@gmail.com>.
> On April 12, 2016, 2:16 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/tests/decoder_tests.cpp, line 202
> > <https://reviews.apache.org/r/46094/diff/1/?file=1341387#file1341387line202>
> >
> > Here and everywhere below: This still leaks if any of above `EXPECT_*` fail; in that case we continue running tests.
> >
> > Since we `ASSERT` that we only got a single response we might as well do just
> >
> > Owned<http::Response> reponse = response[0];
> >
> > If the goal of this nice effort is to be able to run tests under leak checkers (and potentially correlate failures to memory problems), I'd really appreciate using some smart pointer for automatic cleanup instead.
Good point! Adjusted this commit to use `Owned` instead, and submitted an additional RR that makes use of `Owned` in a few other places in the libprocess tests.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/#review128404
-----------------------------------------------------------
On April 12, 2016, 2:02 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46094/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 2:02 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed memory leaks in Encoder/Decoder tests in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
>
> Diff: https://reviews.apache.org/r/46094/diff/
>
>
> Testing
> -------
>
> make check
>
> Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/#review128404
-----------------------------------------------------------
3rdparty/libprocess/src/tests/decoder_tests.cpp (line 202)
<https://reviews.apache.org/r/46094/#comment191828>
Here and everywhere below: This still leaks if any of above `EXPECT_*` fail; in that case we continue running tests.
Since we `ASSERT` that we only got a single response we might as well do just
Owned<http::Response> reponse = response[0];
If the goal of this nice effort is to be able to run tests under leak checkers (and potentially correlate failures to memory problems), I'd really appreciate using some smart pointer for automatic cleanup instead.
- Benjamin Bannier
On April 12, 2016, 4:02 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46094/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 4:02 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed memory leaks in Encoder/Decoder tests in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
>
> Diff: https://reviews.apache.org/r/46094/diff/
>
>
> Testing
> -------
>
> make check
>
> Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests
in libprocess.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46094/#review128490
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45991, 45995, 45996, 45999, 46094]
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 April 12, 2016, 2:02 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46094/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 2:02 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed memory leaks in Encoder/Decoder tests in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51
> 3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b
>
> Diff: https://reviews.apache.org/r/46094/diff/
>
>
> Testing
> -------
>
> make check
>
> Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle.
>
>
> Thanks,
>
> Neil Conway
>
>