Fix missing enqueue notifications in timed_thread_context. - #2231
Fix missing enqueue notifications in timed_thread_context.#2231daniel-schmidt wants to merge 1 commit into
Conversation
The push_back of the command queue can return false during insertion of a new command.
| ready_ = true; | ||
| cv_.notify_one(); | ||
| } | ||
| cv_.notify_one(); |
There was a problem hiding this comment.
@lewissbaker i am remembering a situation in which a cv notify needed to happen within a critical section. is this one of those situations?
There was a problem hiding this comment.
I'm about 80% confident this is not required to be done inside the critical section.
The circumstance where you would need to notify inside the critical section is if the awoken work could cause the destruction of the condition variable before the notify method returns.
In this case, you're notifying the cv with a positive number of inflight submissions, which means, in the worst case, the run function is spinning on that count, waiting for it to go back to zero, which it can't do until notify_one returns. Since the only path to the cv's destruction is through the queue-draining loop in run, and since that loop must be no further advanced than spinning on the count of active submissions, I don't think you can run into UB here.
There was a problem hiding this comment.
The one thing I'd want to check here is whether or not there is a possibility of the run() thread seeing the decrement of n_submissions_in_flight before it sees the effects of notify_one().
The decrement below has relaxed memory order and I don't think that the notify_one member-function provides acquire semantics that would prevent the subsequent access of n_submissions_in_flight_ from being reordered above the notify_one() call. So whether or not this is safe might depend on exactly how the notify_one() call is implemented - I'm not 100% sure on this one.
I would probably keep the call to notify_one() inside the scope of the lock just to be safe here.
EDIT: Actually, on reflection I think the better solution is to make the decrement below a release operation and then the compare-exchange in run() an acquire operation.
|
/ok to test b62e1c4 |
| n_submissions_in_flight_.compare_exchange_strong(n, | ||
| context_closed, | ||
| STDEXEC::__std::memory_order_relaxed); |
There was a problem hiding this comment.
Incidentally to this PR, I don't understand why this CAS is here.
If we're running this code then n < 0 must be true; that can only happen if run has exited the busy-loop that's trying to set n_submissions_in_flight_ to context_closed. Given that run has successfully exited the busy loop, there's nothing I can see that prevents the current context from being destroyed out from under you here. We're already playing with fire by having discovered that *this might be about to be destroyed by dereferencing this on line 214; continuing to dereference this after we know it's in its death throes seems like all downside and no upside.
Maybe there's a concern that, at shut-down, such a large number of attempts to schedule will happen "all at once" that context_closed gets incremented up to 0. I suppose that could theoretically happen and, if it did, it would be disastrous. That could be fixed by changing how "closed" is represented. I'd split n_submissions_in_flight_ into a one-bit "closed?" field in the LSB, and a count of in-flight submissions in the remaining bits; incrementing and decrementing the count could then be done with fetch_add(2, …) and fetch_sub(2, …), and the busy-loop that's trying to mark the context as closed could spinning around compare_exchange_weak(0, 1, …). The check for a negative n here would instead be a check for odd n. It's impossible to accidentally overflow the count field and corrupt the closed field with this strategy.
|
@daniel-schmidt could you please look into the clang-16 test failure? |
| STDEXEC_ASSERT(op->command_ == command_type::command_type::stop); | ||
| static_cast<stop_type*>(op)->set_value_(op); | ||
| } | ||
| n_submissions_in_flight_.compare_exchange_strong(n, |
There was a problem hiding this comment.
Why is this line trying to change 'n' back to 'context_closed'?
The 'n' value obtained above was the value before 1 was added to it, so in the no-contention case this is going to fail (as the value is likely still n+1 rather than n).
This seems like it could potentially just set the value back to 'context_closed' or just decrement the value instead? Or alternatively, turn the above fetch_add into a compare_exchange loop that leaves the value unchanged if it's currently 'context_closed'.
Either way, some commentary here would be helpful for future readers.
| STDEXEC::__spin_loop_pause(); | ||
| expected = 0; | ||
| } | ||
| op = heap_.front(); |
There was a problem hiding this comment.
This loop seems like it could spin for an unbounded amount of time if other threads keep queuing new items onto it as a single other thread could potentially continuously win the race of incrementing the counter to 1, then this loop checks the counter and spins, then the other thread decrements, then immediately enqueues another item (which increments the count to 1).
An alternative would be to atomically set a flag that means that new schedule operations are not permitted once the count reaches zero when the flag is set. This would put a bound on the number of other items that can be enqueued to the number of threads currently calling schedule() at the time that the shutdown begins.
Having said that, at a higher level I would probably try to avoid a situation where the users can still potentially schedule more items once the timed_thread_context destructor starts. I would instead make it a precondition of the destructor that all scheduled items have completed and that no further items will be scheduled onto the context. This would greatly simplify the shutdown approach - once you see the 'stop-requested' flag you just assert that the queue is empty and exit the run() function. You could also then completely get rid of the n_submissions_in_flight_ data-member.
| ready_ = true; | ||
| cv_.notify_one(); | ||
| } | ||
| cv_.notify_one(); |
There was a problem hiding this comment.
The one thing I'd want to check here is whether or not there is a possibility of the run() thread seeing the decrement of n_submissions_in_flight before it sees the effects of notify_one().
The decrement below has relaxed memory order and I don't think that the notify_one member-function provides acquire semantics that would prevent the subsequent access of n_submissions_in_flight_ from being reordered above the notify_one() call. So whether or not this is safe might depend on exactly how the notify_one() call is implemented - I'm not 100% sure on this one.
I would probably keep the call to notify_one() inside the scope of the lock just to be safe here.
EDIT: Actually, on reflection I think the better solution is to make the decrement below a release operation and then the compare-exchange in run() an acquire operation.
4 participants
The push_back of the command queue can return false during insertion of a new command.
See #2230 for details.