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

Pattern for deprecated IOLoop constructor #3156

Closed
minrk opened this issue Jun 14, 2022 · 5 comments · Fixed by #3160
Closed

Pattern for deprecated IOLoop constructor #3156

minrk opened this issue Jun 14, 2022 · 5 comments · Fixed by #3160

Comments

@minrk
Copy link
Contributor

minrk commented Jun 14, 2022

I see in 6.2 beta that the IOLoop constructor is deprecated, and that migration docs are forthcoming. I wanted to report how we've been using one of the deprecated patterns as an example of something that might need migration.

We have a pattern of using tornado to handle some background IO in threads, that looks like:

from tornado.ioloop import IOLoop

class Background(Thread):
    def __init__(self):
        self.io_loop = IOLoop(make_current=False)

    def run(self):
        self.io_loop.start()

    def stop(self):
        self.io_loop.add_callback(self.io_loop.stop)

This pattern of creating the loop and talking to it before it starts is super useful. But the code as-is has started failing with the 6.2 beta (I'm not sure if the breakage is intentional or not), and I see that calling IOLoop() constructor directly is deprecated, which I understand given its relationship to the deprecated asyncio.get_event_loop().

Now, I've found that the following code works, if I explicitly choose the AsyncIOLoop implementation:

from tornado.platform.asyncio import AsyncIOLoop

class Background(Thread):
    def __init__(self):
        self.io_loop = AsyncIOLoop()

    def run(self):
        self.io_loop.start()

    def stop(self):
        self.io_loop.add_callback(self.io_loop.stop)

This makes sense to me, because AsyncIOLoop explicitly calls asyncio.new_event_loop and doesn't rely on any globals like get/set_event_loop. Is this a reasonable approach? Or is creating/accessing any tornado IOLoop while the asyncio loop is not running going to be unsupported, or at least discouraged?

Explicitly creating / having a handle on the event loop is easy enough with asyncio, but if we can't get a handle on the tornado IOLoop object until asyncio is running in the background thread, that's going to be a pain.

@bdarnell
Copy link
Member

Thanks for the feedback and for testing the new beta. It's a bug that the behavior of the current code has changed - the only intentional change is to start emitting deprecation warnings (when those are enabled) in some places. I'll take a look and see why it's breaking.

That's an interesting observation about AsyncIOLoop vs IOLoop. I forgot that class still exists with slightly different semantics from the default. It should be emitting a deprecation warning if make_current=True, but other than that I think we could consider supporting that pattern. Maybe we could even undeprecate the IOLoop constructor with make_current=False and give it the semantics of AsyncIOLoop.

@minrk
Copy link
Contributor Author

minrk commented Jun 14, 2022

Thanks! That makes sense.

Keeping everything working with the 3.10 deprecations is pretty tough, so I don't mind needing updates to do the 'make a loop, configure it, then start it' pattern. I'd just like to do whatever you think is the official way to do that. Certainly, "wrap it all in one big asyncio.run" is one option, but more of a pain to adapt to.

@minrk
Copy link
Contributor Author

minrk commented Jun 15, 2022

btw, I was wrong about the above code working without make_current=False, but AsyncIOLoop(make_current=False) is working: ipython/ipykernel#956

@minrk
Copy link
Contributor Author

minrk commented Jun 15, 2022

After chasing my tail for longer than it should have taken, IOLoop(make_current=False) actually works fine and emits no deprecation warnings. So I think this pattern can be addressed solely by updating the documentation that IOLoop(make_current=False) is not going to be deprecated.

Sorry if you wasted time on this, too!

@bdarnell
Copy link
Member

Great. Looks like the .. deprecated:: 6.2 block in the docs is accurate but the main body of the docs left out this possibility. Based on the commit message I think I had doubts about whether anything but test frameworks would use this option, but you've shown that there is a need so I'll update that paragraph.

bdarnell added a commit to bdarnell/tornado that referenced this issue Jun 17, 2022
The constructor is not completely deprecated; the make_current=False
mode is still usable.

Fixes tornadoweb#3156
bdarnell added a commit to bdarnell/tornado that referenced this issue Jun 17, 2022
The constructor is not completely deprecated; the make_current=False
mode is still usable.

Fixes tornadoweb#3156
bdarnell added a commit to bdarnell/tornado that referenced this issue Jun 17, 2022
The constructor is not completely deprecated; the make_current=False
mode is still usable.

Fixes tornadoweb#3156
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 a pull request may close this issue.

2 participants