Add callback for when a task is added to a run-loop schedule#337
Add callback for when a task is added to a run-loop schedule#337kirkshoop merged 2 commits intoReactiveX:masterfrom studoot:add-runloop-callback
Conversation
|
Hi @studoot, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution! TTYL, MSOTBOT; |
ghost
added
the
| st->r.reset(false); | ||
| std::function<void()> callback{st->add_task_callback}; | ||
| guard.unlock(); // So we can't get attempt to recursively lock the state | ||
| if (callback) callback(); |
There was a problem hiding this comment.
schedule() is called from other threads in order to schedule work on the thread that owns the runloop.
callback() will be called from other threads. It is not okay to dispatch any events from the callback since they would be running on the wrong thread. The only thing that the callback can do is reset the timer to an earlier value if the new item is supposed to occur earlier than the previous timer was set. This would be easier if the callback was passed the new time.
If so, perhaps it would be better if the callback was only called when the new when is earlier than the previous queue top. In that case, the name of the callback might be notify_next_wakeup instead of add_task_callback.
| st->r.reset(false); | ||
| std::function<void()> callback{st->add_task_callback}; | ||
| guard.unlock(); // So we can't get attempt to recursively lock the state | ||
| if (callback) callback(); |
There was a problem hiding this comment.
schedule() is called from threads other than the thread the owns the runloop.
callback() is also called from other threads. The callback must not dispatch runloop items. The only thing the callback could safely do is set a timer to trigger at an earlier time than it currently is set.
passing the new when would make that easier.
only calling the callback if the new time is earlier than the previous top item would be reasonable as well.
If those changes are made then the name of the callback might change as well. Perhaps notify_when_changed?
|
:) I was sure that github had lost my comment - it just disappeared. So I wrote it again! |
|
Hmmmm - without an easy way of signalling the run loop's thread, I think this needs a bit more thought. One thing I was thinking of was to just provide more information about the state of the queue (are there tasks ready for dispatch or newly added since the last dispatch?). Or to document that the callback is purely to wake the run loops thread (I know how to do that with Qt...)... How does that sound? |
I think that passing the I think that a small change to what you have in this PR will create this semantic.
IMO, 'newly added' should not signal the runloops thread. Many times the new item will be in the future, or at the same time as the item already at the front/top of the runloop. Waking the thread to run nothing or waking the thread repeatedly for the same time, when each item is scheduled, would be very chatty. Do you think that you could do what you need in QT with the changes to the callback I proposed? |
Modified callback to provide new wakeup time as a callback parameter, so that it can schedule an event to wake the thread owning the run loop, rather than (as was the case) having to iterate through the dispatch loop.
|
To answer your question - yes, and here's an updated PR... This works with Qt and (if anything) simplifies that code. Rationale for some of the change:
|
|
I am happy that this works well for QT, that gives me confidence. Thank you for this change! |
2 participants
See #154 for motivation for this PR...
Implementation decisions:
std::function<void()>:std::function<void()>{}