Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure execution order of callbacks that are expected to be called immediately (such as call_soon) #123

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gampixi
Copy link

@gampixi gampixi commented Jul 17, 2024

Summary

Ensure execution order of callbacks with zero delay (such as call_soon) by handling them independently of delayed callbacks.

It's reasonable to assume that custom event loops would adhere to the contract established in the asyncio docs. Quoting the asyncio docs:

Callbacks are called in the order in which they are registered.

This PR accomplishes it by running a separate timer (with 0 timeout) for servicing immediate callbacks. According to Qt documentation, this timer will run each time the Qt event loop finishes servicing window events (https://doc.qt.io/qt-6/qtimer.html#interval-prop), which should ensure responsive operation.

Issue was observed on Windows.

I hope someone more versed in asyncio and Qt can chime in here. I see this fix more as a best-guess workaround, as I didn't dive into why the 0-delay timer callbacks would get executed out-of-order in the first place. Some quick research didn't give clear clues on whether we can expect any execution order guarantees from the original method.

Some background

Under heavy UI load (such as real-time plotting with pyqtgraph), it was observed that the zero-delay timers scheduled via _SimpleTimer would occasionally run out-of-order on Windows. In these cases multiple immediate callbacks would be serviced within a single event loop iteration.

Unfortunately I haven't been able to get a minimum reproducible example working despite my best efforts to artificially abuse the event loop, but I'll give some overview on how the issue was observed.

We have an application that reads a realtime data stream from a BLE device (using Bleak). New values may be received up to 400 times per second, with each update causing Bleak to use call_soon to queue the processing of the value.

As observed, each update roughly goes through these stages:

  1. Bleak calls call_soon_threadsafe, which makes Qt emit the passed callback as a signal
  2. The signal callback calls call_soon (from a different thread than Bleak used)
  3. qasync redirects call_soon to call_later with delay=0
  4. call_later builds an asyncio.Handle and passes it to _add_callback
  5. _add_callback redirects to the _SimpleTimer, which registers a timer with a timeout of 0
  6. A short while later, the registered timer timeouts, and is handled by the timerEvent method

The out-of-order issue was observed by adding tracing to each respective stage, where the 6th stage would occasionally call the callbacks out of order, like so: [1, 2, 3, 4, 6, 5, 7, 8, ...]. No callbacks were lost.

This would only happen when the app has heavy load, such as when a graph update took longer than several milliseconds.

After applying the workaround from this PR, the issue no longer happens.

…them independently of delayed callbacks

Under heavy UI load (such as real-time plotting with pyqtgraph), it was observed that the zero-delay timers scheduled via _SimpleTimer would occasionally run out-of-order on Windows.
@gampixi
Copy link
Author

gampixi commented Jul 17, 2024

Hang on, I've rushed with creating this PR a bit. This approach causes excessive CPU usage due to excessive polling. It should be straightforward to mitigate that, though.

Edit: commit 797c8f6 should fix that. The timer now gets created on demand, but the general idea that all immediate callbacks are serviced by a single (rather than separate per-callback) timer remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant