Revert some deprecations, following asyncio changes

[takluyver]

This is an attempt to tackle #3216 - at least the straightforward parts of it.

• Remove deprecation warnings and notices for ioloop make_current and clear_current methods, and the make_current constructor parameter.
• Remove deprecation notices on AsyncTestCase and AsyncHTTPTestCase
• Remove the mention of Python changes from the TCPServer bind & start deprecation notices (but they're still deprecated)

I've assumed that removing the deprecation notices is sufficient documentation - we could add notes saying that they were deprecated and then undeprecated again, but that feels like unnecessary clutter.

Still to figure out:

• IOLoop.current() has a deprecation notice for using it without a running asyncio loop. Is this also un-deprecated? Does it need some code change to handle when there's no running loop nor a default loop specifically set in asyncio?
• Are some changes needed to this code in AsyncTestCase?

  tornado/tornado/testing.py

  Lines 204 to 238 in 5953601

  	# NOTE: this code attempts to navigate deprecation warnings introduced
  	# in Python 3.10. The idea of an implicit current event loop is
  	# deprecated in that version, with the intention that tests like this
  	# explicitly create a new event loop and run on it. However, other
  	# packages such as pytest-asyncio (as of version 0.16.0) still rely on
  	# the implicit current event loop and we want to be compatible with them
  	# (even when run on 3.10, but not, of course, on the future version of
  	# python that removes the get/set_event_loop methods completely).
  	#
  	# Deprecation warnings were introduced inconsistently:
  	# asyncio.get_event_loop warns, but
  	# asyncio.get_event_loop_policy().get_event_loop does not. Similarly,
  	# none of the set_event_loop methods warn, although comments on
  	# https://bugs.python.org/issue39529 indicate that they are also
  	# intended for future removal.
  	#
  	# Therefore, we first attempt to access the event loop with the
  	# (non-warning) policy method, and if it fails, fall back to creating a
  	# new event loop. We do not have effective test coverage of the
  	# new event loop case; this will have to be watched when/if
  	# get_event_loop is actually removed.
  	self.should_close_asyncio_loop = False
  	try:
  	self.asyncio_loop = asyncio.get_event_loop_policy().get_event_loop()
  	except Exception:
  	self.asyncio_loop = asyncio.new_event_loop()
  	self.should_close_asyncio_loop = True
  	async def get_loop() -> IOLoop:
  	return self.get_new_ioloop()
  	self.io_loop = self.asyncio_loop.run_until_complete(get_loop())
  	with warnings.catch_warnings():
  	warnings.simplefilter("ignore", DeprecationWarning)
  	self.io_loop.make_current()

• AnyThreadEventLoopPolicy remains deprecated (as mentioned in Reverse some deprecation warnings from Tornado 6.2 #3216), but is any change needed to its deprecation notice?

  tornado/tornado/platform/asyncio.py

  Lines 423 to 431 in 5953601

  	.. deprecated:: 6.2
  	``AnyThreadEventLoopPolicy`` affects the implicit creation
  	of an event loop, which is deprecated in Python 3.10 and
  	will be removed in a future version of Python. At that time
  	``AnyThreadEventLoopPolicy`` will no longer be useful.
  	If you are relying on it, use `asyncio.new_event_loop`
  	or `asyncio.run` explicitly in any non-main threads that
  	need event loops.

[takluyver]

I think the CI failure is something random unrelated to these changes. It's failing in a test about subprocess handling, only on Python 3.10, and only with LANG=en_US.utf-8. The changes so far shouldn't make any functional difference, except for getting rid of some warnings.

[bdarnell]

Thanks!

These changes look good, but to be sure I want to wait until we can test with the next alpha of python 3.12 (due next week) before merging.

The CI issue is a known flaky test which I've just written up as #3225. We should probably just skip that test since it's not helpful to trip over it every once in a while.

IOLoop.current: Yes, I think we'll need some code changes there to keep the current semantics. Probably something in the except block after get_event_loop that creates a new one if it doesn't exist. Or we could leave this as-is and adopt asyncio's stance against implicit creation of event loops. That would mean in practice that most applications would still want to adopt asyncio.run, so I think it's more important for us to keep backwards compatibility than to track asyncio on this point.

In the test classes, I think we can just revert that whole block from https://github.com/tornadoweb/tornado/pull/3097/files#diff-0c44c25deecc01d7f48e0fd36005e637bb772273e31377f21fb16690c285dc4b - the old code should work again. But this is something we'd need to test with the new alpha.

AnyThreadEventLoopPolicy's deprecation comment is still valid. In general I think we're probably fine with removing the deprecation notices entirely. We'll explain the undeprecations in the 6.3 release notes and amend the ones from 6.2 to note what's changed again.

[takluyver]

Thanks! No problem about waiting for the next 3.12 alpha. From this discussion, it looks like the behaviour changes (not creating a default event loop for the main thread) are being delayed to 3.14, because the deprecation warnings weren't thorough enough.

I've changed IOLoop.current() to create a new event loop if necessary, but only on the main thread. It doesn't seem to be documented that this behaves differently depending on the thread, but there is a test that fails if it doesn't (part of AnyThreadEventLoopPolicyTest checks the behaviour with the default policy). Would you rather I changed the behaviour and the test so IOLoop.current(instance=True) can create a loop on any thread?

In the test classes, I think we can just revert that whole block from https://github.com/tornadoweb/tornado/pull/3097/files#diff-0c44c25deecc01d7f48e0fd36005e637bb772273e31377f21fb16690c285dc4b - the old code should work again.

I'm still a bit confused about this. The comment added in that PR says "packages such as pytest-asyncio (as of version 0.16.0) still rely on the implicit current event loop and we want to be compatible with them". But I think reverting to the code before that will always create a new event loop (calling IOLoop()) and set it as the current one.

It's unfortunate that there appears to be no way of getting the future (Python 3.14) behaviour now - there's no public attribute or method to check if the current thread already has an event loop without potentially creating a new one.

[bdarnell]

The new 3.12 alpha is out and #3230 enables it in CI, so that will be picked up when you rebase/merge this PR.

I've also been discussing the deprecation plans in python/cpython#100160 (comment) to confirm that we're on the right track.

It's unfortunate that there appears to be no way of getting the future (Python 3.14) behaviour now - there's no public attribute or method to check if the current thread already has an event loop without potentially creating a new one.

Guido is open to adding such a method to 3.12. I thought I needed one when I was working on #3230, but figured out it was obsolete. I need to look back at the testing change to see whether we might need one there.

[takluyver]

OK, I've rebased, and all the tests seem to be passing with the new 3.12 alpha.

[bdarnell]

I'm still a bit confused about this. The comment added in that PR says "packages such as pytest-asyncio (as of version 0.16.0) still rely on the implicit current event loop and we want to be compatible with them". But I think reverting to the code before that will always create a new event loop (calling IOLoop()) and set it as the current one.

Yes, this is confusing, and my brief comments left me struggling to piece together what's going on. But the key is to look at GetNewIOLoopTest. Reverting to the older code will call self.get_new_ioloop() every time, which will normally create a new event loop and make it current. But that method can be overridden, and sometimes must be, to allow some surrounding framework (like pytest-asyncio) to initialize an event loop that will be used in the test.

As long as reverting to the previous version of the testing code passes GetNewIOLoopTest, I think that's all we need to do here.

[bdarnell]

Are you removing AssertionError here based on this comment that says AssertionError was only used in Python 3.4?

[takluyver]

Yup, that's what I went on.

[bdarnell]

I don't think we want the main-thread-only behavior here. Tornado has historically allowed event loop creation on any thread (that's what AnyThreadEventLoopPolicy is for), so it's a nice side effect that we can bring back the "any thread" behavior by default at the same time that AnyThreadEventLoopPolicy is being deprecated.

Just remove this commit.

[takluyver]

OK, I've removed that commit, and adjusted AnyThreadEventLoopPolicyTest to match.

[takluyver]

I reverted the code in AsyncTestCase. Then there were some ResourceWarnings being emitted in the tests (which caused the tests to fail by writing to stderr). It turned out that removing a mysterious workaround in test_asyncio_adapter made them pass again, equally mysteriously, at least on my machine.

[takluyver]

Ugh, I'm having a hard time understanding how the current machinery (current, make_current, clear_current) is meant to work without a way to check asyncio for a 'current' event loop. ioloop.current(instance=False) is kind of a lie, because it can still make a new loop, and even ioloop.make_current() may instantiate another asyncio event loop while trying to check if it's the current one.

With the more precise deprecation warning in Python 3.11.1, we could convert DeprecationWarning to an error, catch it the same way as RuntimeError, and interpret it as 'no existing event loop'. But in 3.11.0 (and several 3.10.x versions), this will also stop us getting an explicitly set loop (i.e. set by asyncio.set_event_loop()).

Or we could just silence the deprecation warnings; either for the tests, or for anything calling tornado's ioloop API. 😕

[takluyver]

For now, I've gone back to suppressing the deprecation warning for large parts of the tests.

[bdarnell]

IOLoop.current(make_instance=False) has been quietly deprecated since Tornado 5.0 for exactly this reason:

tornado/tornado/ioloop.py

Lines 256 to 258 in 9ea5cdd

	an alias for this method). ``instance=False`` is deprecated,
	since even if we do not create an `IOLoop`, this method
	may initialize the asyncio loop.

But no matter what that comment says, we still use this in a couple of places internally.

• IOLoop.initialize uses this to support the default of make_current=None, which means to make the new IOLoop current unless there is already one, and as an error check for make_current=True.
• clear_current uses it to restore the previous asyncio event loop. This dates back to ioloop: Setting AsyncIOLoop as current also sets asyncio event loop #2218 and ioloop,gen,asyncio: Improvements to IOLoop GC #2248, notably preceding asyncio's support for get_running_loop.

I'd be willing to lose the error check in initialize. And could we just change the default for make_current to True (or False; I'm not sure which would be more compatible with actual usage of this method today) and get rid of the old None behavior?

clear_current is tricky. Semantically, I'd like to just make it like set_event_loop(None) without worrying about what was there before. But the problem is that then you just leave the old event loop to be GC'd, and it logs a warning about that.

We have the opportunity to add a get_saved_event_loop method to Python 3.12 while it's still in alphas. That would be useful for navigating the deprecation warnings introduced in that version, but unfortunately it wouldn't help us with these trickier semantic problems that still need to be handled in 3.10 and 3.11.

So let's step back a minute. Maybe we don't need to undeprecate everything here. There's no reason for Tornado to have its own make_current and clear_current, so maybe we keep them deprecated and let them do wasteful things like create a new asyncio event loop just to throw it away. The important things to undeprecate are IOLoop.current() when no event loop is running, and AsyncTestCase. If AsyncTestCase did its setup in terms of asyncio.set_event_loop instead of IOLoop.make_current, does that simplify things?

[bdarnell]

If AsyncTestCase did its setup in terms of asyncio.set_event_loop instead of IOLoop.make_current, does that simplify things?

This does appear to work, except that it breaks the poorly-understood pytest-asyncio interactions. Interestingly, the standard library's unittest.IsolatedAsyncioTestCase appears to do the same thing, unconditionally creating a new event loop without the option to pull anything in from a surrounding framework. pytest-asyncio's README has a comment about this that I find confusingly worded:

Note that test classes subclassing the standard unittest library are not supported. Users are advised to use unittest.IsolatedAsyncioTestCase or an async framework such as asynctest.

Does that mean to use IsolatedAsyncioTestCase instead of pytest-asyncio, or that pytest-asyncio works with IsolatedAsyncioTestCase but not with plain unittest.TestCase? I think it's the former; I don't see any tests or examples of pytest-asyncio being used in conjunction with IsolatedAsyncioTestCase. Given that precedent, I'm inclined to just decide that it's not feasible to combine pytest-asyncio with Tornado's AsyncTestCase in this way. (This may get easier since pytest-asyncio appears to have recently switched to a "strict" mode in which it only activates on annotated test methods instead of running on all of them).

[takluyver]

There's no reason for Tornado to have its own make_current and clear_current, so maybe we keep them deprecated and let them do wasteful things like create a new asyncio event loop just to throw it away.

That sounds reasonable to me. Having fewer pieces that deal with mutable global state seems like a good idea. So the suggested replacements would be:

• make_current() -> asyncio.set_event_loop(tornado_loop.asyncio_loop)
• clear_current() -> asyncio.set_event_loop(None) (up to the application whether they deal with the warnings you mentioned or ignore them)

I'd be willing to lose the error check in initialize. And could we just change the default for make_current to True (or False; I'm not sure which would be more compatible with actual usage of this method today) and get rid of the old None behavior?

If we're deprecating the make_current() method, presumably we would also go for make_current=False as the default for initialize, and deprecate make_current=True?

Does that mean to use IsolatedAsyncioTestCase instead of pytest-asyncio, or that pytest-asyncio works with IsolatedAsyncioTestCase but not with plain unittest.TestCase? I think it's the former;

I read it as the latter. I think they're saying that if you prefer to write your tests unittest style (subclassing a TestCase class) but run them with pytest, then you should subclass from one of those options in your test code, and use the pytest-asyncio plugin to run them. This fits with pytest-dev/pytest-asyncio#180 .

I'm inclined to just decide that it's not feasible to combine pytest-asyncio with Tornado's AsyncTestCase in this way. (This may get easier since pytest-asyncio appears to have recently switched to a "strict" mode in which it only activates on annotated test methods

Indeed, that has made things a lot nicer for compatibility.

So the plan is that AsyncTestCase can always create a new IOLoop and set it as the current loop in asyncio? Should it also clear it in tearDown?

[takluyver]

I've had a go at these changes.

I'd be willing to lose the error check in initialize. And could we just change the default for make_current to True (or False

I figured that if we were deprecating IOLoop(make_current=True) anyway, we may as well leave the behaviour as similar as possible, so I didn't remove the error check.

[takluyver]

I just noticed that make_current=True was already deprecated in 6.2; I can split this out in the docstring if you like.

[takluyver]

I think this maintains compatibility with what pytest-asyncio does? At least, it passes the test that appears to be checking this.

It will create an unnecessary event loop when there isn't already a current one. But after the first test has done this, there will be a current event loop, so subsequent tests will only create and tear down the one they're using.

[bdarnell]

This is sneaky - it works in tornado's test suite because we configure deprecation warnings to be errors. But in a default configuration, it will log a warning and there's no good way to suppress it. (This makes me think that we probably need a CI configuration that doesn't have warnings-as-errors, just like we have one that runs in -O mode to ensure we don't rely on the assert statement which can be disabled).

If the pytest-asyncio integration were valuable enough I think we could figure out a way to keep it and deal with the warning, but given the precedent of IsolatedAsyncioTestCase I think we're better off just simplifying it. The person who originally asked for this feature in #2324 says they no longer need it, and I suspect that the reason they needed it in the first place was something idiosyncratic about their application rather than something broadly applicable to all pytest users.

[takluyver]

In this setUp method, we're specifically silencing those deprecation warnings (if triggered by tornado code). As I understand the filtering system, this should hide them for anyone using AsyncTestCase, regardless of what options they set in their own code.

However, if it's no longer needed now, we can simplify the code to just clear the event loop, without storing the previous one. I think that means taking out GetNewIOLoopTest as well.

[bdarnell]

So the suggested replacements would be:

make_current() -> asyncio.set_event_loop(tornado_loop.asyncio_loop)

Yes, that's the most direct translation, although even better would be to make the asyncio loop the top-level object instead of reaching into the IOLoop to get it.

clear_current() -> asyncio.set_event_loop(None) (up to the application whether they deal with the warnings you mentioned or ignore them)

Yes. One other caveat is that in asyncio, you can't completely reset the state of the global event loop. The implicit event loop creation only works if set_event_loop has never been called (this is why you needed to delete a call to set_event_loop(None) earlier in this PR). Setting it to None doesn't bring back the default behavior, so in some cases it might be more compatible to use asyncio.set_event_loop(asyncio.new_event_loop) instead.

If we're deprecating the make_current() method, presumably we would also go for make_current=False as the default for initialize, and deprecate make_current=True?

We want to get away from all the "current" methods as near-equivalents for set_event_loop. But in the case of the IOLoop constructor, we create a new event loop, and the question is whether we call set_event_loop on it. I think that most of the time, that's going to be what you want.

In any case, I think the IOLoop constructor is essentially soft-deprecated - no new code should use it, although it would be too disruptive to remove it (of course we must have some constructor, so by "remove" I mean make it for internal use of IOLoop.current() only). Given that, we want to make the smallest changes we can while working within the constraints of asyncio. The conditional behavior of make_current=None is difficult to preserve, but whether it should be replaced by True or False depends on whether there is usually an active event loop when an IOLoop is constructed. I think there's usually not, which is why I think set_event_loop is probably the right thing to do.

So the plan is that AsyncTestCase can always create a new IOLoop and set it as the current loop in asyncio? Should it also clear it in tearDown?

Yes. We want to follow IsolatedAsyncioTestCase in this respect, and it does set_event_loop(None) in tearDown.

[takluyver]

I'm conscious that you might be spending more time explaining to me what changes should happen than it would take you to make them yourself. Also, I think the timezones mean we're usually taking a day for each exchange of messages.

If you prefer to make the changes directly, go for it. I've ticked the checkbox that lets you push to my branch, or of course you can make a new branch. I started this because it seemed like a relatively straightforward piece of busywork that I could take care of for you. 🙂

[bdarnell]

I'm conscious that you might be spending more time explaining to me what changes should happen than it would take you to make them yourself. Also, I think the timezones mean we're usually taking a day for each exchange of messages.

I'm also conscious of that possibility, but I don't think it's been a problem yet. I appreciate your help here even though it didn't turn out to be a "straightforward piece of busywork". However, now that we're at the finish line I think I'll take over to do the last few cleanups (including a rebase of the commit history). I'm going to remove the silence of deprecation warnings in AsyncTestCase.setUp, deprecate get_new_ioloop, and add a deprecation warning in clear_current.

[bdarnell]

Actually, the commit history looks pretty decent and rebasing anything looks like more trouble than it's worth (and github is not letting me push to your branch), so I'll merge this and do a quick followup PR for the last couple of changes. Thanks again!

[takluyver]

Thank you!