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

Context Manager / set_event_loop & event loop policies #23

Closed
harvimt opened this issue Jan 15, 2015 · 3 comments
Closed

Context Manager / set_event_loop & event loop policies #23

harvimt opened this issue Jan 15, 2015 · 3 comments

Comments

@harvimt
Copy link
Owner

harvimt commented Jan 15, 2015

Relevant Section of the PEP

This i rambly, sorry.

in bug #22 a user seemed to show confusion about the necessity of and proper place to set the event loop. The context manager sort of obscures all this. Additionally, it's not clear whether unsetting the event loop is particularly necessary. I'm struggling to understand how/why

To give some context, some parts of quamash pre-date quamash, namely \@easycallback which was a helper I made to make working with QThreads easier, (but not easy enough, I still had to write "twisted" callback based code). I was mostly using QThreads for asyncio (like making http requests) anyway, so this worked ok. Executors use a context manager to call shutdown and it seemed event loops should to. At least custom ones, but after reading the PEP as it has evolved this is beginning to seem like a presumption about how event loops are used: set, use, reset to defaults. This doesn't seem to be how they're used at all, usually you use one event loop as sort of the master, and then you can go off and do work in the same thread, in a separate process, in a different thread, but have it all call back to the master event loop.

So several questions:

  1. Should the context manager be deprecated, it doesn't violate the PEP, but it's a feature unique (as far as I know) to Quamash.
  2. If we keep the context manager, since it is somewhat useful, should it be the preferred way to set the event loop?
  3. How often will people be using multiple event loops? (using one, then switching back and forth between which one is active). This seems to be an unusual use case. Not one that should be forbidden, but we don't need to work extra hard to make it easy.
  4. Should an event loop policy be implemented. Then the appropriate usage would be to set the event loop policy, instead of setting the event loop directly. This makes using different event loop implementations harder, (but is also not required usage, you could still set/get the event loop manually or use QEventLoop with the standard policy)
  5. A custom event loop policy has the possibility of making QThreadExecutors more useful as you could run coroutines in them, with a per-thread event loop with less. Even more useful, the .execute could check if a coroutine or a regular function is being passed in, and a coroutine could be wrapped in asyncio.async()' (The policy would then need to manage thread affinity)
    see: http://qt-project.org/doc/qt-4.8/threads-qobject.html (tricky but potentially useful)
  6. A custom event loop policy will likely lead to even more platform specific stuff, and is one more moving part that can go wrong. (well the baseclass will need to be different on windows & unix)

Example code:

@asyncio.coroutine
def async_job():
     yield from asyncio.sleep(.1)

def threaded_job():
    local_loop = asyncio.get_event_loop()  # with standard policy, this will raise an Exception
    asyncio.async(async_job())

    local_loop.run_until_complete(async_job())

asyncio.set_event_loop_policy(quamash.QEventLoopPolicy(app))
loop = asyncio.get_event_loop()
with QThreadExecutor(5) as exec:
   yield from loop.run_in_executor(exec, threaded_job)
@harvimt harvimt added this to the future milestone Jan 15, 2015
@harvimt
Copy link
Owner Author

harvimt commented Jan 15, 2015

@aknuds1 said:

It's better to stick with the standard, so I agree, get rid of the context manager installation side effect (providing that this is non-standard behaviour, I haven't double checked).

However, isn't it still a good idea to use the context manager to close the loop after it's done with?

Well, having the context manager and close the loop without setting it seems weird. I was gonna say that it violates the with-statement PEP, but it obviously doesn't since that's exactly how open() and contextlib.closing work. (w/o a builtin context manager, it would still work with contextlib.closing)

I'm not sure that shutting down the event loop is even necessary, unless you're going to start a new one. (which is weird). It will automatically get shut down when the process exits which is fine.

@aknuds1
Copy link
Collaborator

aknuds1 commented Jan 15, 2015

Yeah, I don't understand how having the context manager only close the loop would be weird, since that's typical behaviour. So there are two aspects here I think:

  1. Please keep the context manager, for closing the loop. One can otherwise use contextlib.closing, but it's smoother not having to reach for this.
  2. While it's not necessary to close the loop in the example, I think it's good to promote the pattern of freeing your resources. Sometimes you might open a loop that's going to be disposed before the program ends.

@horazont
Copy link
Contributor

I don’t think that having a QEventLoopPolicy is required. However, it is a nice-to-have and the possible features this would allow definitely sound neat to me. I think a step-by-step approach is appropriate here.

I have no strong opinion on the contextmanager, I don’t use it myself.

my two cents

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

No branches or pull requests

3 participants