Merge lp:~vanvugt/mir/less-like-infinite-loops into lp:mir

[Daniel van Vugt]

Reword some while loops in compositor::Stream that look worryingly
like they could potentially be infinite loops. Only looking farther
do you realise that they are finite by virtue of scheduling always
occurring under the same lock as the loop.

So this isn't required, but it will cause less concern for future
readers and is more efficient anyway.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3898
https://mir-jenkins.ubuntu.com/job/mir-ci/2480/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/3234
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3301
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3293
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3263/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/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3263/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3263/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/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3263/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/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3263/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/3263
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3263/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

The while-loop and the for-loop now don't have the same semantics, depending on whether or not schedule->num_scheduled() could change during iterations.

- while(schedule->num_scheduled())
9 + unsigned n = schedule->num_scheduled();
10 + for (unsigned i = 0; i < n; ++i)

review: Needs Information

[Daniel van Vugt (vanvugt)]

Correct, but what I was trying to say in the commit message was that the locking around the loops stops num_sheduled from ever changing during the loops so for and while are the same here.

It's not a guarantee that can survive all future accidental code changes, but it is guaranteed for now. This change aims to stop the loops from becoming infinite/indefinite loops if anyone ever accidentally changes the locking structure. And this change is a bit more efficient too.

[Alan Griffiths (alan-griffiths)]

The "after" code is less clear than the "before".

review: Disapprove

[Chris Halse Rogers (raof)]

Weak disapprove; this changes the implied intent. I believe the behaviour we intend is “while there are scheduled buffers, do something” rather than “for each buffer scheduled before the loop starts, do something”.

review: Abstain

Unmerged revisions

3898. By Daniel van Vugt on 2016-12-22

Prototype

=== modified file 'src/server/compositor/stream.cpp'
--- src/server/compositor/stream.cpp	2016-11-17 08:45:49 +0000
+++ src/server/compositor/stream.cpp	2016-12-22 10:08:26 +0000
@@ -76,7 +76,8 @@
 
 mc::Stream::~Stream()
 {
-    while(schedule->num_scheduled())
+    unsigned n = schedule->num_scheduled();
+    for (unsigned i = 0; i < n; ++i)
         buffers->send_buffer(schedule->next_buffer()->id());
 }
 
@@ -173,9 +174,10 @@
     std::shared_ptr<mc::Schedule>&& new_schedule, std::lock_guard<std::mutex> const&)
 {
     std::vector<std::shared_ptr<mg::Buffer>> transferred_buffers;
-    while(schedule->num_scheduled())
+    unsigned n = schedule->num_scheduled();
+    for (unsigned i = 0; i < n; ++i)
         transferred_buffers.emplace_back(schedule->next_buffer());
-    for(auto& buffer : transferred_buffers)
+    for (auto& buffer : transferred_buffers)
         new_schedule->schedule(buffer);
     schedule = new_schedule;
     arbiter->set_schedule(schedule);
@@ -198,7 +200,8 @@
 {
     std::lock_guard<decltype(mutex)> lk(mutex); 
     std::vector<std::shared_ptr<mg::Buffer>> transferred_buffers;
-    while(schedule->num_scheduled())
+    unsigned n = schedule->num_scheduled();
+    for (unsigned i = 0; i < n; ++i)
         transferred_buffers.emplace_back(schedule->next_buffer());
 
     if (!transferred_buffers.empty())