A WinForms event loop integration without performance impact

[Oliver-Leigh]

This PR provides a reactive WinForms proactor event loop. It removes the need for a 5ms delay between loop iterations.

To remove the 5ms delay, two issues needed to be solved:

• Wake the loop on GUI events: This is achieved by replacing the self._ready deque with a child class that wakes the loop when an task is appended.
• Wake the loop when IO completion messages are received: The built in function to listen for messages on the I/O completion port (GetQueuedCompletionStatus) is thread blocking and only fires when events are received. The usual proactor event loop polls the IOCP using GetQueuedCompletionStatus, but here GetQueuedCompletionStatus is run on a separate thread and can fire immediately after receiving completion messages.

Draft for CI testing.

Fixes #2613.

PR Checklist:

• I will abide by the BeeWare Code of Conduct
• I have read and have followed the CONTRIBUTING.md file
• This PR was generated or assisted using an AI tool

Assisted-by:

[freakboy3742]

Wow - thanks for this PR! This is another huge win that I've had on my wish list for a while.

Manual testing of this works really well; I'm still trying to wrap my head round some of the details, but I wanted to get you an initial review of the bits I do understand :-)

The primary concern I have is that by introducing as second thread, we could inadvertently introduce a path for GUI events to be generated or processed on a non-GUI thread; or that the two threads could get into a mutually locked state.

It's entirely possible that you've accounted for these problems - I need to spend some more time (re-)familiarising myself with proactors to convince myself. Alternatively, if there's anything you can do to lay out the general architecture as a permanent long-form comment (or better yet - an architecture document) that would be really helpful.

I'll try and take a closer look at this early next week.

[freakboy3742]

This makes sense as reasoning; but is there any way to avoid the problem in the first place? Is there any way to get the GetQueuedCompletionStatus to run on the thread that will allow a clean shutdown without needing to duplicate the body of the method?

To be clear - I'll accept this if it it actually required. It's just a bit messy (and comes with a maintenance overhead; if we can avoid the mess by a bit of restructuring (or even a redundant no-op call somewhere), that seems preferable.

[Oliver-Leigh]

This has been greatly simplified. The original was caused by my misreading of the Microsoft documentation

[Oliver-Leigh]

The primary concern I have is that by introducing as second thread, we could inadvertently introduce a path forGUI events to be generated or processed on a non-GUI thread; or that the two threads could get into a mutually locked state.

It's a good concern to have. However, I don't think either of these two things could happen here. The second thread only listens for the events on the IOCP and then sends the events to the first thread for processing.

I think another area to highlight is the possibility of in-thread reentrancy. This comes from enqueuing multiple ticks with Task.Delay(delay).ContinueWith(self.task) where delay=0, meaning that tasks are called concurrently with the .NET async implementation. In practice this means that .NET will switch between these tasks and will often call a method that is still in progress. That's why I created the ReentrantQueue to make sure that tasks are run one at a time.

Alternatively, if there's anything you can do to lay out the general architecture as a permanent long-form comment (or better yet - an architecture document) that would be really helpful.

I'm happy to make an architecture document. Where would be a good place for that sort of thing?

I'll try and take a closer look at this early next week.

Take your time! I didn't really appreciate until testing the extent to which this impacts the whole toga_winforms backend.

[freakboy3742]

I think another area to highlight is the possibility of in-thread reentrancy. This comes from enqueuing multiple ticks with Task.Delay(delay).ContinueWith(self.task) where delay=0, meaning that tasks are called concurrently with the .NET async implementation. In practice this means that .NET will switch between these tasks and will often call a method that is still in progress. That's why I created the ReentrantQueue to make sure that tasks are run one at a time.

Thanks for the heads up - I'll keep this in mind as I review.

I'm happy to make an architecture document. Where would be a good place for that sort of thing?

There's a topics/internals section in the docs; it's a bit sparse, but I'd say this would be a good match.

[freakboy3742]

Those updates look great - I have a lot fewer concerns about thread locking now. A couple of minor questions and possible cleanups; it's entirely possible they might be a result of my own misunderstandings, though, so feel free to tell me I'm barking up the wrong tree :-)

[freakboy3742]

@Oliver-Leigh I've been running CI on this all day - based on that testing, it looks like the version from ad61037, even if it is "missing the safety catch", is more stable in CI than (apparently) both of our attempts to fix it.

If you're able to work out the source of the instability, I'm up for a fix; but if you can't, I'm just as happy to revert for now, and treat that as a "later" problem.

[freakboy3742]

I realise the one thing I didn't say: This looks great, and other than the CI stability issue around the safety catch, I think it's ready to land.

[Oliver-Leigh]

@Oliver-Leigh I've been running CI on this all day - based on that testing, it looks like the version from ad61037, even if it is "missing the safety catch", is more stable in CI than (apparently) both of our attempts to fix it.

If you're able to work out the source of the instability, I'm up for a fix; but if you can't, I'm just as happy to revert for now, and treat that as a "later" problem.

I'll have a think about the safety catch and let you know what I find. I've found issues with both of our last commits. The potential interactions can get quite complicated. I'm thinking to make a more independent safety catch.

Can you let me know where the tests where failing? Was is it mainly with the dialogs?

[freakboy3742]

Can you let me know where the tests where failing? Was is it mainly with the dialogs?

AFAICT, it wasn't dialogs - it was odd timing errors with the DetailedList or Table widgets, and sometimes issues with the WebView.

You can check the specifics by opening the build details for the CI passes; by default, it will show you the most recent run, but you can see any of the re-runs as well by selecting the run on the top right of screen.

[Oliver-Leigh]

@freakboy3742 I've added a new independent safety catch. It has a bit more code than I'd like, but on the other hand it's easy to see what it does. Local testing seems to work well.

[freakboy3742]

The symmetry between the "normal" tick and the "safety" tick suggests to me that there might be a path to refactoring that code to avoid the duplication... but then, there's only a handful of lines, so maybe the duplication will actually be simpler.

I've run this through the CI gauntlet a bunch of times, and it seems to be resilient (certainly a lot more resilient than my attempt yesterday), so let's call this a win. Thanks for the PR - this is a huge improvement!