From 56a3e7a92eeabbf51fdacd35ff448fc6c0f6775d Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 29 Apr 2016 08:56:30 -0400
Subject: [PATCH] Make round_robin::suspend_until() always set asio timer.

This gives notify() something to cancel if need be.

Avoid resetting the timer to the same abs_time, though.
---
 examples/asio/round_robin.hpp | 83 ++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 20 deletions(-)

diff --git a/examples/asio/round_robin.hpp b/examples/asio/round_robin.hpp
index 805db416..7b17a936 100644
--- a/examples/asio/round_robin.hpp
+++ b/examples/asio/round_robin.hpp
@@ -50,23 +50,25 @@ public:
             boost::asio::io_service::service( io_svc),
             work_{ new boost::asio::io_service::work( io_svc) } {
             io_svc.post([&io_svc](){
-                            while ( ! io_svc.stopped() ) {
-                                if ( boost::fibers::has_ready_fibers() ) {
-                                    // run all pending handlers in round_robin
-                                    while ( io_svc.poll() );
-                                    // run pending (ready) fibers
-                                    this_fiber::yield();
-                                } else {
-                                    // run one handler inside io_service
-                                    // if no handler available, block this thread
-                                    if ( ! io_svc.run_one() ) {
-                                        break;
-                                    }
-                                }
-                            }
-                        });
+                while ( ! io_svc.stopped() ) {
+                    if ( boost::fibers::has_ready_fibers() ) {
+                        // run all pending handlers in round_robin
+                        while ( io_svc.poll() );
+                        // run pending (ready) fibers
+                        this_fiber::yield();
+                    } else {
+                        // run one handler inside io_service
+                        // if no handler available, block this thread
+                        if ( ! io_svc.run_one() ) {
+                            break;
+                        }
+                    }
+                }
+            });
         }
 
+        virtual ~service() {}
+
         service( service const&) = delete;
         service & operator=( service const&) = delete;
 
@@ -78,7 +80,10 @@ public:
     round_robin( boost::asio::io_service & io_svc) :
         io_svc_( io_svc),
         suspend_timer_( io_svc_) {
-        boost::asio::use_service< service >( io_svc_);
+        // We use add_service() very deliberately. This will throw
+        // service_already_exists if you pass the same io_service instance to
+        // more than one round_robin instance.
+        boost::asio::add_service( io_svc_, new service( io_svc_));
     }
 
     void awakened( context * ctx) noexcept {
@@ -104,15 +109,53 @@ public:
     }
 
     void suspend_until( std::chrono::steady_clock::time_point const& abs_time) noexcept {
-        if ( (std::chrono::steady_clock::time_point::max)() != abs_time) {
+        // Set a timer so at least one handler will eventually fire, causing
+        // run_one() to eventually return. Set a timer even if abs_time ==
+        // time_point::max() so the timer can be canceled by our notify()
+        // method -- which calls the handler.
+        if ( suspend_timer_.expires_at() != abs_time) {
+            // Each expires_at(time_point) call cancels any previous pending
+            // call. We could inadvertently spin like this:
+            // dispatcher calls suspend_until() with earliest wake time
+            // suspend_until() sets suspend_timer_
+            // lambda loop calls run_one()
+            // some other asio handler runs before timer expires
+            // run_one() returns to lambda loop
+            // lambda loop yields to dispatcher
+            // dispatcher finds no ready fibers
+            // dispatcher calls suspend_until() with SAME wake time
+            // suspend_until() sets suspend_timer_ to same time, canceling
+            // previous async_wait()
+            // lambda loop calls run_one()
+            // asio calls suspend_timer_ handler with operation_aborted
+            // run_one() returns to lambda loop... etc. etc.
+            // So only actually set the timer when we're passed a DIFFERENT
+            // abs_time value.
             suspend_timer_.expires_at( abs_time);
-            suspend_timer_.async_wait([](boost::system::error_code const&){
-                                        this_fiber::yield();
-                                      });
+            // It really doesn't matter what the suspend_timer_ handler does,
+            // or even whether it's called because the timer ran out or was
+            // canceled. The whole point is to cause the run_one() call to
+            // return. So just pass a no-op lambda with proper signature.
+            suspend_timer_.async_wait([](boost::system::error_code const&){});
         }
     }
 
     void notify() noexcept {
+        // Something has happened that should wake one or more fibers BEFORE
+        // suspend_timer_ expires. Reset the timer to cause it to fire
+        // immediately, causing the run_one() call to return. In theory we
+        // could use cancel() because we don't care whether suspend_timer_'s
+        // handler is called with operation_aborted or success. However --
+        // cancel() doesn't change the expiration time, and we use
+        // suspend_timer_'s expiration time to decide whether it's already
+        // set. If suspend_until() set some specific wake time, then notify()
+        // canceled it, then suspend_until() was called again with the same
+        // wake time, it would match suspend_timer_'s expiration time and we'd
+        // refrain from setting the timer. So instead of simply calling
+        // cancel(), reset the timer, which cancels the pending sleep AND sets
+        // a new expiration time. This will cause us to spin the loop twice --
+        // once for the operation_aborted handler, once for timer expiration
+        // -- but that shouldn't be a big problem.
         suspend_timer_.expires_at( std::chrono::steady_clock::now() );
     }
 };