Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Ensure get_event_loop returns the running loop when called in a coroutine #355

Closed
wants to merge 18 commits into from
Closed

Ensure get_event_loop returns the running loop when called in a coroutine #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b30bcdc
Modify the interface for AbstractEventLoopPolicy
May 25, 2016
705a54c
Update BaseEventLoop.run_forever to set and clear the running loop
May 25, 2016
2e55a60
Ignore event loop policy warnings during the tests
May 25, 2016
f117c75
Fix SleepTests and TimeoutTests
May 25, 2016
1c3a166
Revert "Ignore event loop policy warnings during the tests"
Jun 3, 2016
6e8eaa2
Remove running loop context and event loop policy warnings
Jun 3, 2016
be2c0f1
Revert "Fix SleepTests and TimeoutTests"
Jun 3, 2016
c1cb666
Update docstrings for AbstractEventLoopPolicy
Jun 3, 2016
8788c72
Remove get/set_default_loop
Jun 3, 2016
121cca3
Add disable_get_running_loop method to TestCase
Jun 3, 2016
f1674e8
Update set_event_loop docstring
Jun 3, 2016
c634f79
Merge both try-finally blocks into one in BaseEventLoop.run_forever
Jun 3, 2016
71fcfab
Fix new_event_loop docstring for AbstractEventLoopPolicy
Jun 4, 2016
1a60ee8
Add test_set_running_loop to PolicyTests
Jun 4, 2016
db6774d
Clean set_running_loop
Jun 4, 2016
28c3de3
Add test_get_event_loop_after_set_running_loop to PolicyTests
Jun 4, 2016
5b19055
Fix get_event_loop docstring
Jun 4, 2016
7513577
Update running context in BaseEventLoop.run_forever
Jun 4, 2016
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
3 changes: 3 additions & 0 deletions asyncio/base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,16 +339,19 @@ def run_forever(self):
self._check_closed()
if self.is_running():
raise RuntimeError('Event loop is running.')
policy = events.get_event_loop_policy()
self._set_coroutine_wrapper(self._debug)
self._thread_id = threading.get_ident()
try:
policy.set_running_loop(self)
Copy link
Member

Choose a reason for hiding this comment

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

This has to be outside of the try statement.

Copy link
Author

Choose a reason for hiding this comment

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

If we do that, this test (not commited yet) would fail:

    def test_run_loop_inside_loop(self):

        @asyncio.coroutine
        def coro():
            loop2 = asyncio.new_event_loop()
            self.assertRaises(RuntimeError, loop2.run_forever)
            self.assertFalse(loop2.is_running())  # AssertionError: True is not false
            loop2.close()

        loop = asyncio.new_event_loop()
        loop.run_until_complete(coro())
        loop.close()

Copy link
Member

Choose a reason for hiding this comment

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

I'm beginning to think that this is all one big misunderstanding. I never
meant get_event_loop() to return a loop that's different from the running
loop. If you have multiple loops associated with the same thread you need
to implement a new EventloopPolicy that keeps track of which one is running
(e.g. via an explicit stack, or perhaps an implicit one using a local
variable to save the event loop and restoring from that in a finally
clause).

The only thing I meant to be possible is for get_event_loop() to raise an
exception, under special circumstances (especially unit tests that are
checking that no code accidentally relies on the default loop).

For library code I think there are two cases it should support:

  • No default event loop, loop must always passed in
  • Default event loop == current event loop, used when no loop is passed in

For application code I think it is totally fine to assume that
get_event_loop() returns the current, running loop. An application written
with this assumption never needs to pass a loop to something it calls
(since library code should always default to using get_event_loop()).

Note all the words devoted in the PEP to "context".

In any case I think we should stop coding and start discussing the use case
you are trying to address on the tulip list, to see whether it's real or
whether you are worrying too much (or whether maybe you should just write a
custom EventloopPolicy for your use case).

Copy link
Author

@vxgmichel vxgmichel Jun 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@gvanrossum
I actually don't have a use case. I simply noticed that asyncio, 3rd party libraries and users tend to pass the running loop explicitely to their coroutines, because "explicit is better than implicit". That, in my opinion, is unecessary and could be simplified.

Sadly, such a simplification conflicts with the PEP and the way asyncio unittesting currently works. I understand that those constraints makes it hard, and maybe impossible to implement. I orginally meant this PR as a proposal and a starting point towards this simplification.

However, this idea didn't seem to get a lot of support and I don't feel like trying to push it forward anymore. Maybe more people will come up with the same rationale later on, and I'll be happy to help at that time.

Copy link
Member

Choose a reason for hiding this comment

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

@gvanrossum

I'm beginning to think that this is all one big misunderstanding. I never
meant get_event_loop() to return a loop that's different from the running
loop. If you have multiple loops associated with the same thread you need
to implement a new EventloopPolicy that keeps track of which one is running
(e.g. via an explicit stack, or perhaps an implicit one using a local
variable to save the event loop and restoring from that in a finally
clause).

But here's the problem: if you have multiple loops associated with the same thread, policy doesn't know which one is currently running.

That's why I like the idea of adding policy.set_running_loop and policy.get_running_loop functions, because it makes policies more capable and allows you to worry less about multiple running loops event with the default policy.

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 have multiple loops associated with the same thread, policy doesn't know which one is currently running.

Then write a new policy. The whole point of having policies is that you can write different ones. The default policy does not really cater to this use case, but a policy is a really simple object.

Copy link
Member

Choose a reason for hiding this comment

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

Without set_running_loop and get_running_loop how will the policy know precisely what's going on? All I'm saying is that get_event_loop and set_event_loop are kind of detached from the loop's lifecycle; policy doesn't receive any notification if a loop was stopped or run again.

Copy link
Member

Choose a reason for hiding this comment

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

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
policy.set_running_loop(None)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call policy.set_running_loop right after self._thread_id = None

self._set_coroutine_wrapper(False)

def run_until_complete(self, future):
Expand Down
66 changes: 55 additions & 11 deletions asyncio/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,21 +514,44 @@ class AbstractEventLoopPolicy:
def get_event_loop(self):
"""Get the 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.
Returns an event loop object implementing the BaseEventLoop interface:
- the running loop if it has been set (using set_running_loop)
- the loop for the current context otherwise.

It should never return None."""
It may also raise 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.
"""
raise NotImplementedError

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

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
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."""
"""Create and return a new event loop object.

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

# Child processes handling (Unix only).
Expand Down Expand Up @@ -559,16 +582,22 @@ class BaseDefaultEventLoopPolicy(AbstractEventLoopPolicy):

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.
"""Get the event loop for the current context.

This may be None or an instance of EventLoop.
Returns an event loop object implementing the BaseEventLoop interface:
- the running loop if it has been set (using set_running_loop)
- the loop for the current thread otherwise.
"""
running_loop = self.get_running_loop()
if running_loop is not None:
return running_loop
if (self._local._loop is None and
not self._local._set_called and
isinstance(threading.current_thread(), threading._MainThread)):
Expand All @@ -579,11 +608,26 @@ def get_event_loop(self):
return self._local._loop

def set_event_loop(self, loop):
"""Set the event loop."""
"""Set the 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 •
edited
Loading

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)
running_loop = self._local._running_loop
if running_loop is not None and loop is not None:
raise RuntimeError('A loop is already running')
self._local._running_loop = loop

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

Expand Down
16 changes: 16 additions & 0 deletions asyncio/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,9 +403,25 @@ def get_function_source(func):


class TestCase(unittest.TestCase):
def disable_get_event_loop(self):
policy = events.get_event_loop_policy()
if hasattr(policy, '_patched_get_event_loop'):
return

def reset_event_loop_method():
policy.get_running_loop = old_get_running_loop
del policy._patched_get_event_loop

old_get_running_loop = policy.get_running_loop
policy.get_running_loop = lambda: None
policy._patched_get_event_loop = True

self.addCleanup(reset_event_loop_method)

def set_event_loop(self, loop, *, cleanup=True):
assert loop is not None
# ensure that the event loop is passed explicitly in asyncio
self.disable_get_event_loop()
events.set_event_loop(None)
if cleanup:
self.addCleanup(loop.close)
Expand Down
51 changes: 51 additions & 0 deletions tests/test_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2467,6 +2467,9 @@ def test_event_loop_policy(self):
self.assertRaises(NotImplementedError, policy.get_event_loop)
self.assertRaises(NotImplementedError, policy.set_event_loop, object())
self.assertRaises(NotImplementedError, policy.new_event_loop)
self.assertRaises(NotImplementedError, policy.get_running_loop)
self.assertRaises(NotImplementedError, policy.set_running_loop,
object())
self.assertRaises(NotImplementedError, policy.get_child_watcher)
self.assertRaises(NotImplementedError, policy.set_child_watcher,
object())
Expand Down Expand Up @@ -2534,6 +2537,54 @@ def test_set_event_loop(self):
loop.close()
old_loop.close()

def test_set_running_loop(self):
policy = asyncio.DefaultEventLoopPolicy()
self.assertIsNone(policy._local._running_loop)
self.assertIsNone(policy.get_running_loop())

self.assertRaises(AssertionError, policy.set_running_loop, object())

loop = policy.new_event_loop()
policy.set_running_loop(loop)

self.assertIs(policy._local._running_loop, loop)
self.assertIs(policy.get_running_loop(), loop)
loop.close()

loop2 = policy.new_event_loop()
self.assertRaises(RuntimeError, policy.set_running_loop, loop2)
loop2.close()

policy.set_running_loop(None)
self.assertIsNone(policy._local._running_loop)

def test_get_event_loop_after_set_running_loop(self):
policy = asyncio.DefaultEventLoopPolicy()

running_loop = policy.new_event_loop()
policy.set_running_loop(running_loop)

self.assertIsNone(policy._local._loop)
self.assertIs(policy.get_event_loop(), running_loop)

loop = policy.new_event_loop()
policy.set_event_loop(loop)

self.assertIs(policy._local._loop, loop)
self.assertIs(policy.get_event_loop(), running_loop)

policy.set_running_loop(None)
running_loop.close()

self.assertIs(policy._local._loop, loop)
self.assertIs(policy.get_event_loop(), loop)

policy.set_event_loop(None)
loop.close()

self.assertIsNone(policy._local._loop)
self.assertRaises(RuntimeError, policy.get_event_loop)

def test_get_event_loop_policy(self):
policy = asyncio.get_event_loop_policy()
self.assertIsInstance(policy, asyncio.AbstractEventLoopPolicy)
Expand Down