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

PeriodicCallback: support async/coroutine callback #2924

Merged
merged 7 commits into from
May 30, 2021

Conversation

ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Sep 28, 2020

#2832 plus some tests


EDIT: nevermind, figured out how to avoid this

ORIGINAL:

I can't quite figure out how to quiet this error log:

[E 200927 21:24:05 ioloop:910] Exception in callback <function TestPeriodicCallbackAsync.test_periodic_coro.<locals>.callback at 0x10a2a6c80>
    Traceback (most recent call last):
      File "/Users/pierce/scratch/tornado/tornado/ioloop.py", line 908, in _run
        await val  # type: ignore
    concurrent.futures._base.CancelledError

The usual pattern doesn't work:

        with ExpectLog(app_log, "Exception in callback"):
            self.wait()

even if I extend it to:

        with ExpectLog(app_log, "Exception in callback"):
            pc = PeriodicCallback(callback, 10)
            pc.start()
            self.wait()
            pc.stop()

@ploxiln ploxiln force-pushed the async_periodic_callback branch 3 times, most recently from 4fd0ead to 641f589 Compare September 29, 2020 03:18
@ploxiln ploxiln force-pushed the async_periodic_callback branch from 641f589 to 673f8cd Compare October 30, 2020 23:21
@ploxiln
Copy link
Contributor Author

ploxiln commented Oct 31, 2020

rebased on master, latest CI matrix ran

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

A couple of type annotation issues here. I thought we had enough type inferencing enabled in tests to catch this but I guess not. Adding a -> None to the test_ methods might enable this.

tornado/ioloop.py Outdated Show resolved Hide resolved
tornado/ioloop.py Outdated Show resolved Hide resolved
@ploxiln ploxiln force-pushed the async_periodic_callback branch 2 times, most recently from 91e96e2 to 74c78d2 Compare November 3, 2020 04:14
@ploxiln
Copy link
Contributor Author

ploxiln commented Nov 3, 2020

btw, on macOS I get one more unrelated mypy warning:

tornado/netutil.py:117: error: Tuple index out of range

and it makes sense why it does not trigger in linux or windows modes:

        if (
            sys.platform == "darwin"
            and address == "localhost"
            and af == socket.AF_INET6
            and sockaddr[3] != 0
        ):
            # Mac OS X includes a link-local address fe80::1%lo0 in the
            # getaddrinfo results for 'localhost'.  However, the firewall
            # ...

@ploxiln ploxiln force-pushed the async_periodic_callback branch from e806eb5 to 30af70b Compare May 29, 2021 19:50
@ploxiln
Copy link
Contributor Author

ploxiln commented May 29, 2021

rebased

self, when: float, callback: Callable[..., None], *args: Any, **kwargs: Any
self,
when: float,
callback: Callable[..., Optional[Awaitable]],
Copy link
Member

Choose a reason for hiding this comment

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

call_at and call_later should have the same type signature. And I think that since they don't do anything with the return value of their argument, they should actually follow the example of add_callback and take an unspecialized Callable. (or equivalently, Callable[..., Any], which is what asyncio uses)

@@ -863,11 +868,14 @@ class PeriodicCallback(object):

.. versionchanged:: 5.1
The ``jitter`` argument is added.

.. versionchanged:: 6.2
The ``callback`` argument may be a coroutine.
Copy link
Member

Choose a reason for hiding this comment

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

The change is more subtle than that: This PR adds support for native coroutines. Decorated coroutines have always been supported by the code (I think; haven't tested it. The mypy annotations would reject them but I'm pretty sure they'd still work). However, this changes their semantics: they were previously fire-and-forget (so a slow coroutine could end up with multiple copies of the task running concurrently) while now they affect scheduling (so a slow coroutine will reduce the number of calls made per unit of time).

This is a potentially breaking change so we don't want to make it by accident. I think it's probably the right move but it needs to be a deliberate decision and documented accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Native coroutines worked too, I used them for a long time, but they were fire-and-forget as you say. I'll update this to be more specific.

@@ -694,6 +694,60 @@ def test_timedelta(self):
self.assertEqual(pc.callback_time, expected_callback_time)


class TestPeriodicCallbackAsync(AsyncTestCase):
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 want tests that cover the overrun behavior (since this is the part that's changing for decorated coroutines between this change and the prior implementation)

@bdarnell
Copy link
Member

Thanks!

@bdarnell bdarnell merged commit a9fbbee into tornadoweb:master May 30, 2021
ploxiln added a commit to ploxiln/tornado that referenced this pull request May 31, 2021
ploxiln added a commit to ploxiln/tornado that referenced this pull request May 31, 2021
jeyrce pushed a commit to jeyrce/tornado that referenced this pull request Aug 25, 2021
ISSUE: tornadoweb#2828

* ioloop: call_later() and call_at() take any Callable coroutine or plain, returning any type

Co-authored-by: agnewee <agnewee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants