potentially throwing `run_loop::finish` can cause `sync_wait` to call `terminate` or hang

[ericniebler]

run_loop::finish puts the run_loop into the finishing state so that the next time the work queue is empty, run_loop::run will return instead of waiting for more work.

calling .finish() on a run_loop instance can potentially throw (finish() is not marked noexcept). that is because one valid implementation involves acquiring a lock on a std::mutex -- a potentially throwing operation.

but failing to put the run_loop into the finishing state is problematic in the same way that a failing destructor is problematic: shutdown and clean-up code depends on it succeeding.

consider sync_wait's use of run_loop:

sync-wait-state<Sndr> state;
auto op = connect(sndr, sync-wait-receiver<Sndr>{&state});
start(op);

state.loop.run();
if (state.error) {
  rethrow_exception(std::move(state.error));
}
return std::move(state.result);

it is the job of sync-wait-receiver to put the run_loop into the finishing state so that the invocation of state.loop.run() will return. it does that in its completion functions, like so:

void set_stopped() && noexcept;

Effects: Equivalent to state->loop.finish().

here we are not handling the fact that state->loop.finish() is potentially throwing. given that this function is noexcept, this will lead to the application getting terminated. not good.

but even if we handle the exception and save it into state.result to be rethrown later, we still have a problem. since run_loop::finish() threw, the run_loop has not been placed into the finishing state. that means that state.loop.run() will never return, and sync_wait will hang forever.

simply put, run_loop::finish() has to be noexcept. the implementation must find a way to put the run_loop into the finishing state. if it cannot, it should terminate. throwing an exception and foisting the problem on the caller -- who has no recourse -- is simply wrong.

Proposed Resolution:

Change run_loop::finish() no-throw. in [exec.run.loop.general], change the definition of the run_loop class as follows:

 namespace std::execution {
   class run_loop {
     // [exec.run.loop.types], associated types
     class run-loop-scheduler;                                   // exposition only
     class run-loop-sender;                                      // exposition only
     struct run-loop-opstate-base {                              // exposition only
       virtual void execute() = 0;                               // exposition only
       run_loop* loop;                                           // exposition only
       run-loop-opstate-base* next;                              // exposition only
     };
     template<class Rcvr>
       using run-loop-opstate = unspecified;                     // exposition only
 
     // [exec.run.loop.members], member functions
     run-loop-opstate-base* pop-front();                         // exposition only
     void push-back(run-loop-opstate-base*);                     // exposition only
 
   public:
     // [exec.run.loop.ctor], constructor and destructor
     run_loop() noexcept;
     run_loop(run_loop&&) = delete;
     ~run_loop();
 
     // [exec.run.loop.members], member functions
     run-loop-scheduler get_scheduler();
     void run();
-    void finish();
+    void finish() noexcept;
   };
 }

change [exec.run.loop.mempers] p8 as follows:

-void finish();
+void finish() noexcept;

8. Preconditions: state is either starting or running.
9. Effects: Changes state to finishing.
10. Synchronization: finish synchronizes with the pop-front operation that returns nullptr.

[lewissbaker]

Do we also need to mark run() as noexcept?
Or at least documented as "Throws: Nothing"?

Currently if run() were to throw (which can only happen if pop-front() were to throw), it's preconditions would preclude calling run() again to drain any remaining schedule tasks.