Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,10 +342,11 @@ def run_forever(self):
self._set_coroutine_wrapper(self._debug)
self._thread_id = threading.get_ident()
try:
while True:
self._run_once()
if self._stopping:
break
with self._running_context():
while True:
self._run_once()
if self._stopping:
break
finally:
Copy link
Member

Choose a reason for hiding this comment

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

I've left a comment somewhere in this PR on how to merge two try-finally blocks into one. Please do that.

self._stopping = False
self._thread_id = None
Expand Down
112 changes: 101 additions & 11 deletions asyncio/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import sys
import threading
import traceback
import warnings
import contextlib

from asyncio import compat

Expand Down Expand Up @@ -507,28 +509,52 @@ def get_debug(self):
def set_debug(self, enabled):
raise NotImplementedError

# Running context

def _running_context(self):
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need this as a separate method, this is something that users shouldn't ever call directly (even though it's a "private" name they will).

Copy link
Author

Choose a reason for hiding this comment

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

Right, it is probably safer!

return get_event_loop_policy().running_loop_context(self)


class AbstractEventLoopPolicy:
"""Abstract policy for accessing the event loop."""

def get_event_loop(self):
"""Get the event loop for the current context.
def get_default_loop(self):
"""Get the default event loop for the current context.

Returns an event loop object implementing the BaseEventLoop interface,
or raises an exception in case no event loop has been set for the
current context and the current policy does not specify to create one.

It should never return None."""
It should never return None.
"""
raise NotImplementedError

def set_default_loop(self, loop):
"""Set the default event loop for the current context to loop."""
raise NotImplementedError

def set_event_loop(self, loop):
"""Set the event loop for the current context to loop."""
def get_running_loop(self):
"""Get the running event loop running for the current context, if any.

Returns an event loop object implementing the BaseEventLoop interface.
If no running loop is set, it returns None.
"""
raise NotImplementedError

def set_running_loop(self, loop):
"""Set the running event loop for the current context.

The loop argument can be None to clear the former running loop.
This method should be called by the event loop itself to set the
running loop when it starts, and clear it when it's done.
"""
raise NotImplementedError

def new_event_loop(self):
"""Create and return a new event loop object according to this
Copy link
Member

Choose a reason for hiding this comment

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

Please fix this docstring too, while you're here.

policy's rules. If there's need to set this loop as the event loop for
the current context, set_event_loop must be called explicitly."""
policy's rules. If there's need to set this loop as the default loop
for the current context, set_event_loop must be called explicitly.
"""
raise NotImplementedError

# Child processes handling (Unix only).
Expand All @@ -541,6 +567,37 @@ def set_child_watcher(self, watcher):
"""Set the watcher for child processes."""
raise NotImplementedError

# Non-abstract methods

def get_event_loop(self):
"""Return the running event loop if any, and the default event
Copy link
Member

Choose a reason for hiding this comment

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

The first docstring line should be one-line sentence under 72 characters long (see https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings). Please fix all docstrings that this PR adds/modifies.

loop otherwise.
"""
running_loop = self.get_running_loop()
if running_loop is not None:
return running_loop
return self.get_default_loop()

def set_event_loop(self, loop):
"""Set the default event loop if the former loop is not currently
running.
"""
if self.get_running_loop() is not None:
raise RuntimeError('The former loop is currently running')
self.set_default_loop(loop)

@contextlib.contextmanager
def running_loop_context(self, loop):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this. get_running_loop and set_running_loop is enough.

Copy link
Author

Choose a reason for hiding this comment

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

My idea here was to keep BaseEventLoop.run_forever simple, and make it easier for other event loop implementations. I can remove it if you think it's not worth it. This is how run_forever looks like without the context manager:

    def run_forever(self):
        """Run until stop() is called."""
        self._check_closed()
        if self.is_running():
            raise RuntimeError('Event loop is running.')
        self._set_coroutine_wrapper(self._debug)
        self._thread_id = threading.get_ident()
        try:
            policy = get_event_loop_policy()
            policy.set_running_loop(self)
            try:
                while True:
                    self._run_once()
                    if self._stopping:
                        break
            finally:
                policy.set_running_loop(None)
        finally:
            self._stopping = False
            self._thread_id = None
            self._set_coroutine_wrapper(False)

Copy link
Member

Choose a reason for hiding this comment

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

If you re-use the outer try-except, it's not going to be that ugly, and it's still easy to use for alternative implementations (can't see any problems to add this to uvloop, for instance). I'd really prefer to keep the number of new APIs to the minimum, so please remove the running_loop_context manager.

Copy link
Author

Choose a reason for hiding this comment

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

All right I'll remove it. However I don't think it is possible to re-use the outer try-except since policy.set_running_loop might raise an exception and prevent the run to be cleaned up correctly.

Copy link
Member

Choose a reason for hiding this comment

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

However I don't think it is possible to re-use the outer try-except since policy.set_running_loop might raise an exception and prevent the run to be cleaned up correctly.

Well, just put it in the right place:

finally:
    self._stopping = False
    self._thread_id = None
    policy.set_running_loop(None)
    self._set_coroutine_wrapper(False)

Let's have just one try-finally block.

"""Convenience context to set and clear the given loop as running
loop. It is meant to be used inside the loop run methods.
"""
assert isinstance(loop, AbstractEventLoop)
self.set_running_loop(loop)
try:
yield
finally:
self.set_running_loop(None)


class BaseDefaultEventLoopPolicy(AbstractEventLoopPolicy):
"""Default policy implementation for accessing the event loop.
Expand All @@ -556,16 +613,23 @@ class BaseDefaultEventLoopPolicy(AbstractEventLoopPolicy):
"""

_loop_factory = None
_warnings = True

class _Local(threading.local):
_loop = None
_running_loop = None
_set_called = False

def __init__(self):
self._local = self._Local()

def get_event_loop(self):
"""Get the event loop.
def warn(self, *args):
if self._warnings:
raise RuntimeError(*args)
warnings.warn(*args)

def get_default_loop(self):
"""Get the default event loop for the current thread.

This may be None or an instance of EventLoop.
"""
Expand All @@ -578,12 +642,38 @@ def get_event_loop(self):
% threading.current_thread().name)
return self._local._loop

def set_event_loop(self, loop):
"""Set the event loop."""
def set_default_loop(self, loop):
"""Set the default event loop for the current thread."""
self._local._set_called = True
assert loop is None or isinstance(loop, AbstractEventLoop)
self._local._loop = loop

def get_running_loop(self):
"""Get the running event loop for the current thread if any.

This may be None or an instance of EventLoop.
"""
return self._local._running_loop
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to return None or raise an exception here?

Copy link
Author

@vxgmichel vxgmichel Jun 3, 2016

Choose a reason for hiding this comment

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

I think None is better for different reasons. The most obvious is this one:

policy.set_running_loop(None)             # Clear the running loop
assert policy.get_running_loop() == None  # This test is expected to pass

Copy link
Member

Choose a reason for hiding this comment

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

Agree, let's keep the current behaviour (returning None).


def set_running_loop(self, loop):
"""Set the running event loop for the current thread."""
assert loop is None or isinstance(loop, AbstractEventLoop)
default_loop = self._local._loop
Copy link
Member

Choose a reason for hiding this comment

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

default_loop isn't used

running_loop = self._local._running_loop
if running_loop is not None and loop is not None:
raise RuntimeError('A loop is already running')
# Warnings
if loop is not None:
if default_loop is None:
self.warn(
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a warning? Might be an annoying thing in unittests.

'Running a loop with no default loop set',
RuntimeWarning)
elif loop != default_loop:
self.warn(
Copy link
Member

Choose a reason for hiding this comment

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

And why is this a problem?

Copy link
Author

@vxgmichel vxgmichel Jun 3, 2016

Choose a reason for hiding this comment

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

My idea here is to take profit of the policy mecanism to make the default policy a bit stricter than required, in order to encourage users to keep a one-to-one mapping between threads and loops. For instance:

# OK
loop = asyncio.get_event_loop()
loop.run_until_complete(coro())
# OK
loop = CustomEventLoop()
asyncio.set_event_loop(loop)
loop.run_until_complete(coro())
# Warning
loop = asyncio.new_event_loop()
loop.run_until_complete(coro())

Maybe warnings are not the best solution, or maybe it is not worth it at all.
Do you know any use case for which using a loop without setting it makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe warnings are not the best solution, or maybe it is not worth it at all.

I'd say this is worth thinking about, but not for 3.5.2. Let's make this PR as simple as possible.

Do you know any use case for which using a loop without setting it makes sense?

unittests mainly, where people frequently call set_event_loop(None) in unittest.setUp to make sure that the loop is passed explicitly everywhere in their library. As for the real code - I don't know, I've never used more than one loop in one process.

'Running a loop different from the default loop',
RuntimeWarning)
self._local._running_loop = loop

def new_event_loop(self):
"""Create a new event loop.

Expand Down
4 changes: 4 additions & 0 deletions asyncio/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ def set_event_loop(self, loop, *, cleanup=True):
assert loop is not None
# ensure that the event loop is passed explicitly in asyncio
events.set_event_loop(None)
# Disable event loop policy warnings
events.get_event_loop_policy()._warnings = False
Copy link
Member

Choose a reason for hiding this comment

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

So this is how you fix the warnings in tests. The problem is that this is certainly acceptable for asyncio, but not for third party code. Since I'm not sure I understand the utility of these warnings, I'd just remove them.

if cleanup:
self.addCleanup(loop.close)

Expand All @@ -417,6 +419,8 @@ def new_test_loop(self, gen=None):

def tearDown(self):
events.set_event_loop(None)
# Enable event loop policy warnings
events.get_event_loop_policy()._warnings = True

# Detect CPython bug #23353: ensure that yield/yield-from is not used
# in an except block of a generator
Expand Down
12 changes: 2 additions & 10 deletions tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2235,11 +2235,7 @@ def test_run_coroutine_threadsafe_task_factory_exception(self):
class SleepTests(test_utils.TestCase):
def setUp(self):
self.loop = asyncio.new_event_loop()
asyncio.set_event_loop(None)

def tearDown(self):
self.loop.close()
self.loop = None
self.set_event_loop(self.loop)

def test_sleep_zero(self):
result = 0
Expand All @@ -2263,11 +2259,7 @@ def coro():
class TimeoutTests(test_utils.TestCase):
def setUp(self):
self.loop = asyncio.new_event_loop()
asyncio.set_event_loop(None)

def tearDown(self):
self.loop.close()
self.loop = None
self.set_event_loop(self.loop)

def test_timeout(self):
canceled_raised = [False]
Expand Down