1
=== modified file 'src/client/buffer_vault.cpp'
2
--- src/client/buffer_vault.cpp	2017-07-17 02:44:34 +0000
3
+++ src/client/buffer_vault.cpp	2017-07-27 02:04:12 +0000
4
@@ -80,24 +80,63 @@
5
80
mcl::BufferVault::~BufferVault()
80
mcl::BufferVault::~BufferVault()
6
81
{
81
{
7
82
    buffer_factory->cancel_requests_with_context(this);
82
    buffer_factory->cancel_requests_with_context(this);
8
83
9
84
    std::vector<std::shared_ptr<MirBuffer>> current_buffers;
10
85
11
83
    std::unique_lock<std::mutex> lk(mutex);
86
    std::unique_lock<std::mutex> lk(mutex);
12
87
13
88
    // Prevent callbacks from allocating new buffers
14
89
    being_destroyed = true;
15
90
16
84
    for (auto& it : buffers)
91
    for (auto& it : buffers)
18
85
    try
92
    if (auto map = surface_map.lock())
19
86
    {
93
    {
30
87
        if (auto map = surface_map.lock())
94
        if (auto buffer = map->buffer(it.first))
31
88
        {
95
        {
32
89
            if (auto buffer = map->buffer(it.first))
96
            /*
33
90
            {
97
             * Annoying wart:
34
91
                buffer->set_callback(ignore_buffer, nullptr);
98
             *
35
92
            }
99
             * mir::AtomicCallback is atomic via locking, and we need it to be because
36
93
        } 
100
             * we rely on the guarantee that after set_callback() returns no thread
37
94
        if (!disconnected_)
101
             * is in, or will enter, the previous callback.
38
95
        {
102
             *
39
96
            free_buffer(it.first);
103
             * However!
40
104
             *
41
105
             * wire_transfer_inbound() is called from buffer->callback, and acquires
42
106
             * BufferVault::mutex.
43
107
             *
44
108
             * This destuctor must call buffer->set_callback(), and has acquired
45
109
             * BufferVault::mutex.
46
110
             *
47
111
             * So we've got a lock inversion.
48
112
             *
49
113
             * We can't unlock here, as wire_transfer_inbound() might mutate
50
114
             * BufferVault::buffers, which we're iterating over.
51
115
             *
52
116
             * So store them up to process later...
53
117
             */
54
118
            current_buffers.push_back(buffer);
55
119
56
120
            /*
57
121
             * ...and erase them from the SurfaceMap.
58
122
             *
59
123
             * After this point, the RPC layer will ignore any messages about
60
124
             * this buffer.
61
125
             *
62
126
             * We don't need to explicitly ask the server to free it; it'll be
63
127
             * freed with the BufferStream
64
128
             */
65
129
            map->erase(it.first);
66
97
        }
130
        }
67
98
    }
131
    }
69
99
    catch (...)
132
    lk.unlock();
70
133
71
134
    /*
72
135
     * Now, ensure that no threads are in the existing buffer callbacks...
73
136
     */
74
137
    for (auto const& buffer : current_buffers)
75
100
    {
138
    {
76
139
        buffer->set_callback(ignore_buffer, nullptr);
77
101
    }
140
    }
78
102
}
141
}
79
103
142
80
@@ -246,6 +285,14 @@
81
246
void mcl::BufferVault::wire_transfer_inbound(int buffer_id)
285
void mcl::BufferVault::wire_transfer_inbound(int buffer_id)
82
247
{
286
{
83
248
    std::unique_lock<std::mutex> lk(mutex);
287
    std::unique_lock<std::mutex> lk(mutex);
84
288
85
289
    /*
86
290
     * The BufferVault is in the process of being destroyed; there's no point in
87
291
     * doing any processing and we should *certainly* not allocate any more buffers.
88
292
     */
89
293
    if (being_destroyed)
90
294
        return;
91
295
92
249
    last_received_id = buffer_id;
296
    last_received_id = buffer_id;
93
250
    auto buffer = checked_buffer_from_map(buffer_id);
297
    auto buffer = checked_buffer_from_map(buffer_id);
94
251
    auto inbound_size = buffer->size();
298
    auto inbound_size = buffer->size();
95
252
299
96
=== modified file 'src/client/buffer_vault.h'
97
--- src/client/buffer_vault.h	2017-05-08 03:04:26 +0000
98
+++ src/client/buffer_vault.h	2017-07-27 02:04:12 +0000
99
@@ -96,6 +96,7 @@
100
96
    int const usage;
96
    int const usage;
101
97
97
102
98
    std::mutex mutex;
98
    std::mutex mutex;
103
99
    bool being_destroyed{false};
104
99
    BufferMap buffers;
100
    BufferMap buffers;
105
100
    std::deque<NoTLSPromise<std::shared_ptr<MirBuffer>>> promises;
101
    std::deque<NoTLSPromise<std::shared_ptr<MirBuffer>>> promises;
106
101
    geometry::Size size;
102
    geometry::Size size;
107
102
103
108
=== modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
109
--- tests/unit-tests/client/test_buffer_vault.cpp	2017-05-17 04:48:46 +0000
110
+++ tests/unit-tests/client/test_buffer_vault.cpp	2017-07-27 02:04:12 +0000
111
@@ -156,16 +156,6 @@
112
156
    make_vault();
156
    make_vault();
113
157
}
157
}
114
158
158
115
159
TEST_F(BufferVault, frees_the_buffers_we_actually_got)
116
160
{
117
161
    EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
118
162
    EXPECT_CALL(mock_requests, free_buffer(package2.buffer_id()));
119
163
    auto vault = make_vault();
120
164
121
165
    vault->wire_transfer_inbound(package.buffer_id());
122
166
    vault->wire_transfer_inbound(package2.buffer_id());
123
167
}
124
168
125
169
TEST_F(BufferVault, withdrawing_and_never_filling_up_will_timeout)
159
TEST_F(BufferVault, withdrawing_and_never_filling_up_will_timeout)
126
170
{
160
{
127
171
    using namespace std::literals::chrono_literals;
161
    using namespace std::literals::chrono_literals;
128
@@ -370,24 +360,6 @@
129
370
    }, std::exception);
360
    }, std::exception);
130
371
}
361
}
131
372
362
132
373
TEST_F(BufferVault, makes_sure_rpc_calls_exceptions_are_caught_in_destructor)
133
374
{
134
375
    EXPECT_CALL(mock_requests, free_buffer(_))
135
376
        .Times(1)
136
377
        .WillOnce(Throw(std::runtime_error("")));
137
378
    EXPECT_NO_THROW({
138
379
        auto vault = make_vault();
139
380
        vault->wire_transfer_inbound(package.buffer_id());
140
381
    });
141
382
}
142
383
143
384
TEST_F(StartedBufferVault, skips_free_buffer_rpc_calls_if_disconnected)
144
385
{
145
386
    EXPECT_CALL(mock_requests, free_buffer(_))
146
387
        .Times(0);
147
388
    vault.disconnected();
148
389
}
149
390
150
391
TEST_F(StartedBufferVault, buffer_count_remains_the_same_after_scaling)
363
TEST_F(StartedBufferVault, buffer_count_remains_the_same_after_scaling)
151
392
{
364
{
152
393
    std::array<mp::Buffer, 3> buffers;
365
    std::array<mp::Buffer, 3> buffers;
Status:	Merged
Approved by:	Alan Griffiths on 2017-07-27
Approved revision:	no longer in the source branch.
Merged at revision:	4215
Proposed branch:	lp:~raof/mir/fix-lock-inversion
Merge into:	lp:mir
Diff against target:	152 lines (+60/-40) 3 files modified src/client/buffer_vault.cpp (+59/-12) src/client/buffer_vault.h (+1/-0) tests/unit-tests/client/test_buffer_vault.cpp (+0/-28)
To merge this branch:	bzr merge lp:~raof/mir/fix-lock-inversion
Related bugs:	Link a bug report
Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2017-07-27
Alan Griffiths		2017-07-26	Approve on 2017-07-26
Review via email: mp+328069@code.launchpad.net