Mir

Merge lp://staging/~vanvugt/mir/fix-1295231 into lp://staging/mir

fix-1295231
Merge into development-branch

Proposed by Daniel van Vugt on 2014-03-24

Status:

Merged

Approved by:

Alan Griffiths on 2014-03-24

Approved revision:

no longer in the source branch.

Merged at revision:

1500

Proposed branch:

lp://staging/~vanvugt/mir/fix-1295231

Merge into:

lp://staging/mir

Diff against target:

39 lines (+16/-2)

2 files modified

src/client/mir_client_library.cpp (+11/-2)
tests/acceptance-tests/test_protobuf.cpp (+5/-0)

To merge this branch:

bzr merge lp://staging/~vanvugt/mir/fix-1295231

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-24
Alan Griffiths		2014-03-24	Approve on 2014-03-24
Alexandros Frantzis (community)			Approve on 2014-03-24
Review via email: mp+212367@code.staging.launchpad.net

Commit message

Fix client memory leak that is causing sporadic CI test failures in and around
TestClientInput/DemoPrivateProtobuf. (LP: #1295231)

The leak was a simple mistake in the client library. It was triggered by
DemoPrivateProtobuf tests being strangely racy (not fixed here). However
valgrind doesn't detect that as a leak until the next multi-process test
completes. So it would fail typically during TestClientInput.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-24:

8 + // Use a unique_ptr to ensure connection gets deleted even if we have
9 + // thrown an exception (LP: #1295231)
10 + std::unique_ptr<MirConnection> ptr(connection);

This is the wrong place/way to fix the problem.

This code implements a C API and should not be propagating exceptions.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-24:

Alan,

We're not propagating exceptions, for that reason. See the catch() below that.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-24:

> Alan,
>
> We're not propagating exceptions, for that reason. See the catch() below that.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1150/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-24:

Looks good.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-24:

How does the latest iteration fix a memory leak?

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-24:

> How does the latest iteration fix a memory leak?

Sorry, that was a think-o.

While the test may expose raciness in the RPC code I don't think the test itself is the problem. AFAICS the race is between the client posting disconnect and the client detecting that the server has disconnected.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-24:

Rebuilding to see if CI is really happy with this merged.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24: