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

Create wx.lib.asyncio to enable coroutine support #1103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gzxu
Copy link

@gzxu gzxu commented Dec 8, 2018

(Actually this is a new feature) Fixes #1102

Adding wx.BindAsync() and other supporting classes to enable coroutine support for wxPython.

Before the introduction of coroutines, developers for GUI applications have to ways to handle time-consuming tasks, periodically calling wx.Yield(), or making use of bare wx.CallAfter() with threads and callbacks. These methods are either inefficient or too complex, as described in the doc-string in this patch.

Coroutines are introduced to solve this problem. The program logic can be written in one or few coroutines, managing all the required states in the application, without worrying about blocking the main thread. Actually, coroutines are state-machines generated by the compiler (or exactly the Python interpreter). Additionally, as all coroutines (all coroutines launched in the main event loop, exactly) will be executed in the main thread, there will be no worry about any conflict caused by concurrency in the program logic.

Copy link
Member

@RobinD42 RobinD42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the inline comments:

  • There should be a statement in the module docstring about the versions of Python this is compatible with.
  • Classes and methods (at least those intended to be called by the user of this module) need to have docstrings.
  • There needs to be a short statement in CHANGES.rst about this module. Put it at the end of the 4.0.4 section.
  • Please add some unit tests in unittests/test_lib_asyncio.py.


Description
===========
Before the introduction of coroutines, developers for GUI applications have to ways
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to --> two

Description
===========
Before the introduction of coroutines, developers for GUI applications have to ways
to handle time-consuming tasks. One of them is ``wx.Yield()`` which periodically
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's periodic only if you call wx.Yield periodically. You can just remove "periodically" here

===========
Before the introduction of coroutines, developers for GUI applications have to ways
to handle time-consuming tasks. One of them is ``wx.Yield()`` which periodically
yield the control to wxPython and let events to be processed. This is straitforward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yield --> yields

control to wxPython --> control to the event loop


"""

#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line. It only has effect as the first line in the file, and probably should not be in library modules anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the shebang should only reference itself on the firstline. Remove this otherwise you could have a possible security division/injection issue.
if True: run() proves this as theta can be compiled into a "interpreter"

@sander76
Copy link

@gzxu Do you still have intentions finishing this ? I would be really interested in using this asyncio functionality.

@gzxu
Copy link
Author

gzxu commented Jan 26, 2019

@sander76 I am currently using it in my own proprietary projects, but I don't have time to write comments or documentation

@sander76
Copy link

@gzxu @RobinD42 Want to add that I found wxasync .
Works like a charm !

@LeonarddeR
Copy link

I'd be also pretty interested in having this finished. The free open source screen reader NVDA relies on wx, and we won't be able to utilize asyncio effectively if there is no support for it within phoenix.

@sander76
Copy link

I'd be also pretty interested in having this finished. The free open source screen reader NVDA relies on wx, and we won't be able to utilize asyncio effectively if there is no support for it within phoenix.

I use wxasync now. Works perfectly. Would that be a solution for you ?

@Metallicow
Copy link
Contributor

If and when someone gets around to reviewing this...
In addition a demo should be made also and check for python versions, intentional oopsies, possible vulnerabilities(documentation could be one), etc...
Documenting this might be the hardest for some as it could be quite abstract, and while some folks are better coders, they may not be the best at documenting their code. So a few different use cases would be welcomed.

@LeonarddeR
Copy link

I'd be also pretty interested in having this finished. The free open source screen reader NVDA relies on wx, and we won't be able to utilize asyncio effectively if there is no support for it within phoenix.

I use wxasync now. Works perfectly. Would that be a solution for you ?

I'm not particularly happy with the gui message polling approach. It also introduces an extra dependency.

@gzxu
Copy link
Author

gzxu commented Oct 13, 2019

I'm not going to finish the remaining issues in a short time due to my lack of time. Would there be anyone continue on that? cc @RobinD42 @Metallicow

@Metallicow
Copy link
Contributor

@gzxu Most of our lives can be attributed to a short time. As far as mine or anyone elses, I cannot speak.
All I can say for now is that someone might pick it up in their spare time and finish it,
or someone will contact someone who thinks they know will finish it and maybe pay them to do so,
or someone real bright will show up to work drunk, steal the book, and the rest is history.
...and maybe send robin a tip or too...

@Metallicow
Copy link
Contributor

If 'you' was a human calculus experiment, maybe you would understand.

@RobinD42
Copy link
Member

This pull request has been mentioned on Discuss wxPython. There might be relevant details there:

https://discuss.wxpython.org/t/button-events-in-poll-mode/34217/9

@LeonarddeR
Copy link

I'm happy to try applying some of the requested changes. @gzxu What would be the best way to do so? I could of course file a pr against your fork, but given your lack of time, that might still be a problem for you?

@RobinD42
Copy link
Member

One way to do it is to start with @gzxu's branch, add your changes, and then submit as a new PR with a note that links it to this PR.

LeonarddeR pushed a commit to LeonarddeR/Phoenix that referenced this pull request Oct 31, 2019
@LeonarddeR
Copy link

One way to do it is to start with @gzxu's branch, add your changes, and then submit as a new PR with a note that links it to this PR.

That makes sense. I forked phoenix and made a new branch. I've already applied the inline comments in LeonarddeR@be461ff and will try to address the others later. First I need to dive into the world of asyncio again

@LeonarddeR
Copy link

I've applied most of the requests in my asyncio branch. Due to several reasons however, I'm reluctant to file this as a pr.

  1. The process of creating a new event loop based on asyncio.AbstractEventLoop is poorly documented. I'm pretty sure that the current implementation of this branch only supports the basic subset of asyncio, no sockets, no pipes, etc. I've been looking at subclassing asyncio.BaseEventLoop, but at least the python 3.5 and 3.6 docs strongly discourage so as the API is unstable. However, asyncio.BaseEventLoop seems to implement some very fundamental stuff that's currently not in the WxEventLoop code. For example, asyncio.events._set_running_loop is called to set the currently running loop, but that seems to be py3.7 plus. Being compatible with multiple versions of Python as well as maintaining this code will therefore be pretty hard to do.
  2. I'm a beginner in asynchronous programming with asyncio and really feel myself uncomfortable with taking responsibility for code that I'm unable to maintain or even unable to assess the full validity of.

My advise would be that this work either has to be continued bby a core dev, or not at all. It is just too complex for someone who isn't fully fused with WxPython development and asyncio.

I wonder whether @sirk390 would be interested in sharing is findings about his approach in wxasync. I assume he came up with his hybrid solution for a reason.

@sirk390
Copy link

sirk390 commented Nov 2, 2019

Yes, it's a complicated topic, and I also spend days navigating the source code to get my version to work.

The problem here is that this implementation doesn't support network, which is a considerable limitation (most programs that use asyncio require network). And I think it will be complicated to add and maintain.

There would be a better approach, which is to patch the python asyncio event loop to detect GUI messages, and then wake up. I did this by patching "GetQueuedCompletionStatus" to call "MsgWaitForMultipleObjectsEx".
You can see my poc source code here: https://github.com/sirk390/wxasync/blob/master/test/poc_windows_patch_iocp.py (only works on windows)
However, after testing it, it was actually slower than my wxasync polling implementation. I then decided not to spend more time on it, but it could be interesting to know what goes wrong.

One small thing I didn't see in your implementation and that I like a lot, is to link the lifetime of a coroutine to a "wxWindow". That is, you cancel coroutines when the window is destroyed, and only allow BindAsync on wxWindow objects. This avoids a lot of manual work of canceling a Coroutine when you close the window, and avoids a lot of errors/warnings from asyncio or wxpython).
It also enforces better the principle of causality of Nathaniel Smith as explained here: https://vorpus.org/blog/some-thoughts-on-asynchronous-api-design-in-a-post-asyncawait-world/

Some other issues that are complicated (but most of the time not a big issue) are that the event loop can be frozen:
* When you resize a window.
* When you call some system modal dialogs. This could be solved by reimplementing these dialogs

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

Successfully merging this pull request may close these issues.

Integrate with the asyncio module for async and await coroutines support [WITH "PATCH"]
6 participants