Mir

Merge lp:~cemil-azizoglu/mir/fix-1628794 into lp:mir

Proposed by Cemil Azizoglu on 2016-11-16
Status: Work in progress
Proposed branch: lp:~cemil-azizoglu/mir/fix-1628794
Merge into: lp:mir
Diff against target: 60 lines (+16/-12)
1 file modified
src/client/buffer_stream.cpp (+16/-12)
To merge this branch: bzr merge lp:~cemil-azizoglu/mir/fix-1628794
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2016-11-18
Alexandros Frantzis (community) Needs Fixing on 2016-11-18
Andreas Pokorny (community) Approve on 2016-11-18
Daniel van Vugt 2016-11-16 Needs Information on 2016-11-18
Review via email: mp+311082@code.launchpad.net

Commit message

Description of the change

This is preventing async-render-surface from passing CI.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3826
https://mir-jenkins.ubuntu.com/job/mir-ci/2193/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2838/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2903
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2895
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2895
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2895
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2867/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2867/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2867/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2867/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2867/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2867
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2867/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2867/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2193/rebuild

review: Needs Fixing (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

This approach will probably fail (and it did when I tried it some months ago already). The requests complete asynchronously so can't share the same result object. Each call (not just each function) needs its own result object. Also I have further work on the way that uses the protobuf_void.error() values so will need to be able to get at them.

Clearly if the shared_ptr is still leaking then we've got a problem with the NewCallback itself getting leaked at the lower level, sometimes.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

BTW, the leak itself only makes CI fail once a week or so (see bug 1628794).

If the same leak is consistently breaking your branches then maybe you've accidentally touched on the root cause of the leak, just in your branches?

lp:~cemil-azizoglu/mir/fix-1628794 updated on 2016-11-17
3827. By Cemil Azizoglu on 2016-11-17

Don't let response be part of the object that's prone to destruction.

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3827
https://mir-jenkins.ubuntu.com/job/mir-ci/2202/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2919
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2911
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2911
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2911
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2883/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2883/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2883/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2883/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2883/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2883
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2883/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2202/rebuild

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

Isn't this now just a more complicated version of what we already have? Or have you subtly fixed the leak somehow?

review: Needs Information
Cemil Azizoglu (cemil-azizoglu) wrote :

> Isn't this now just a more complicated version of what we already have? Or
> have you subtly fixed the leak somehow?

Yes I have fixed the problem - with this branch integrated, the async-render-surface branch now passes CI (https://code.launchpad.net/~cemil-azizoglu/mir/async-render-surface/+merge/310375/comments/806935).

Previously it triggered the leak every time. It's pretty subtle - it's about the lifetime of the object the responses are contained in. The object is destroyed before the responses are deleted. This MP prevents it from happening. Also, code similar to this (fixed version) is also used elsewhere (e.g. take a look at HandleErrorVoid and handle_structured_error in MirConnection).

Andreas Pokorny (andreas-pokorny) wrote :

there is an inline comment..

What happens with IgnoreResponse if the reply never arrives?

review: Needs Information
lp:~cemil-azizoglu/mir/fix-1628794 updated on 2016-11-18
3828. By Cemil Azizoglu on 2016-11-18

Change per anpok's suggestion.

Cemil Azizoglu (cemil-azizoglu) wrote :

> there is an inline comment..

Changed it accordingly.

>
> What happens with IgnoreResponse if the reply never arrives?

I guess the right thing would be to purge the outsanding replies after a timeout. If that were an issue, however, we would have seen it already as the exact code exists in our codebase - take a look at HandleErrorVoid and handle_structured_error in MirConnection. So with the march towards CapnProto, I don't think it's big enough a concern to warrant spending effort at this point.

Andreas Pokorny (andreas-pokorny) wrote :

ok

review: Approve
Alexandros Frantzis (afrantzis) wrote :

Haven't looked deeply enough to comment on the need for this (vs using shared_ptr).

In any case, we don't need the extra allocation for mp::Void. The following should work (but I haven't tested):

auto const protobuf_void = new mp::Void; // ...shudder...
server.allocate_buffers(&request, protobuf_void,...)

review: Needs Fixing
Alexandros Frantzis (afrantzis) wrote :

> Haven't looked deeply enough to comment on the need for this (vs using shared_ptr).

Having said that, it does feel like this MP is somehow working around the issue instead of fixing its root cause.

lp:~cemil-azizoglu/mir/fix-1628794 updated on 2016-11-18
3829. By Cemil Azizoglu on 2016-11-18

change per alf's comment

3830. By Cemil Azizoglu on 2016-11-18

bah

Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3828
https://mir-jenkins.ubuntu.com/job/mir-ci/2215/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2871/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2936
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2928
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2928
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2928
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2900/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2900/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2900/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2215/rebuild

review: Needs Fixing (continuous-integration)
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3830
https://mir-jenkins.ubuntu.com/job/mir-ci/2217/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2873
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2938
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2930
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2930
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2930
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2902/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2217/rebuild

review: Approve (continuous-integration)

Unmerged revisions

3830. By Cemil Azizoglu on 2016-11-18

bah

3829. By Cemil Azizoglu on 2016-11-18

change per alf's comment

3828. By Cemil Azizoglu on 2016-11-18

Change per anpok's suggestion.

3827. By Cemil Azizoglu on 2016-11-17

Don't let response be part of the object that's prone to destruction.

3826. By Cemil Azizoglu on 2016-11-16

Use paranthesis

3825. By Cemil Azizoglu on 2016-11-16

Fix lp:1628794

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/buffer_stream.cpp'
2--- src/client/buffer_stream.cpp 2016-11-17 08:45:49 +0000
3+++ src/client/buffer_stream.cpp 2016-11-18 15:45:19 +0000
4@@ -53,6 +53,11 @@
5
6 namespace
7 {
8+static void ignore_response(mp::Void* response)
9+{
10+ delete response;
11+}
12+
13 class Requests : public mcl::ServerBufferRequests
14 {
15 public:
16@@ -72,9 +77,10 @@
17 buf_params->set_pixel_format(format);
18 buf_params->set_buffer_usage(usage);
19
20- auto protobuf_void = std::make_shared<mp::Void>();
21- server.allocate_buffers(&request, protobuf_void.get(),
22- google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
23+ auto response = new mp::Void;
24+
25+ server.allocate_buffers(&request, response,
26+ google::protobuf::NewCallback(ignore_response, response));
27 }
28
29 void free_buffer(int buffer_id) override
30@@ -83,9 +89,10 @@
31 request.mutable_id()->set_value(stream_id);
32 request.add_buffers()->set_buffer_id(buffer_id);
33
34- auto protobuf_void = std::make_shared<mp::Void>();
35- server.release_buffers(&request, protobuf_void.get(),
36- google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
37+ auto response = new mp::Void;
38+
39+ server.release_buffers(&request, response,
40+ google::protobuf::NewCallback(ignore_response, response));
41 }
42
43 void submit_buffer(mcl::MirBuffer& buffer) override
44@@ -94,13 +101,10 @@
45 request.mutable_id()->set_value(stream_id);
46 request.mutable_buffer()->set_buffer_id(buffer.rpc_id());
47
48- auto protobuf_void = std::make_shared<mp::Void>();
49- server.submit_buffer(&request, protobuf_void.get(),
50- google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
51- }
52+ auto response = new mp::Void;
53
54- static void ignore_response(std::shared_ptr<mp::Void>)
55- {
56+ server.submit_buffer(&request, response,
57+ google::protobuf::NewCallback(ignore_response, response));
58 }
59
60 private:

Subscribers

People subscribed via source and target branches