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

websocket: Async version of WebSocketHandler.close is needed #2914

Open
benediamond opened this issue Sep 14, 2020 · 6 comments
Open

websocket: Async version of WebSocketHandler.close is needed #2914

benediamond opened this issue Sep 14, 2020 · 6 comments

Comments

@benediamond
Copy link

benediamond commented Sep 14, 2020

NOTE: this is different from #2763. possibly related to #2448, but also distinct.

After starting a websocket server, I am getting

ERROR:asyncio:Task was destroyed but it is pending!
task: <Task pending name='Task-5' coro=<RequestHandler._execute() running at /usr/local/lib/python3.8/site-packages/tornado/web.py:1703> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x10acb2970>()]> cb=[_HandlerDelegate.execute.<locals>.<lambda>() at /usr/local/lib/python3.8/site-packages/tornado/web.py:2333]>

even after appropriately closing all connections. this is different from #2763, where the unresolved task was WebSocketProtocol13._receive_frame_loop, and not all connections were closed.

to reproduce this easily, run the following tiny python3 program:

import asyncio
from abc import ABC
import tornado.websocket
import tornado.httpserver
import tornado.ioloop
import tornado.web

handlers = []
class Handler(tornado.websocket.WebSocketHandler, ABC):
    def open(self):  # making this async and awaiting write_message doesn't silence the tornado warnings.
        handlers.append(self)
    def on_message(self, message):
        pass
    def on_close(self):
        pass
    def check_origin(self, origin):
        return True
application = tornado.web.Application([
    (r'/', Handler),
])
http_server = tornado.httpserver.HTTPServer(application)
loop = tornado.ioloop.IOLoop.current()
async def my_function():  # closes the connections, stops the server, stops the loop after 20s.
    await asyncio.sleep(20.0)
    for handler in handlers:
        handler.close()
    http_server.stop()
    loop.stop()
http_server.listen(8080)
loop.add_callback(my_function)
loop.start()
loop.close()

in a separate Node.js console (for example), run:

const WebSocket = require('ws');
new WebSocket('ws://localhost:8080')

the culprit appears to be the future fut here, which is never awaited:

tornado/tornado/web.py

Lines 2323 to 2326 in 79b9c4f

fut = gen.convert_yielded(
self.handler._execute(transforms, *self.path_args, **self.path_kwargs)
)
fut.add_done_callback(lambda f: f.result())

tornado version: tornado-6.0.4. Python version: 3.8.5.

i am unable to further diagnose the issue, or fix it. thanks for your attention and time.

@benediamond
Copy link
Author

@garenchan sorry to bother—not sure if you have any thoughts on this. thanks.

@ebrightfield
Copy link

Also having this problem. Do you know where might a safe (and close-to-the-source) place be to catch this with a try/except block until this is fixed?

@infime
Copy link

infime commented Oct 12, 2020

Also running into this after upgrading from python 3.7.6 to 3.8.6

@AlanW0ng
Copy link

AlanW0ng commented Jan 6, 2022

Also running into this after upgrading from python 3.7.6 to 3.8.6

Thanks. I solved this problem by downgrading python to 3.6

@graingert
Copy link
Contributor

you can avoid this by using an asyncio.Event() to wait for the on_close( method to be called

Here I created an async def aclose( method, which you can gather when shutting down your server gracefully:

import functools
import asyncio
import tornado.websocket
import tornado.httpserver
import tornado.ioloop
import tornado.web


class Handler(tornado.websocket.WebSocketHandler):
    def initialize(self, *, handlers):
        self._handlers = handlers
        self._closed = asyncio.Event()

    def open(self):
        self._handlers.add(self)

    def on_message(self, message):
        pass

    def on_close(self):
        self._handlers.remove(self)
        self._closed.set()

    def check_origin(self, origin):
        return True

    async def aclose(self):
        self.close()
        await self._closed.wait()
        return self.close_code, self.close_reason


async def my_function():
    handlers = set()
    application = tornado.web.Application(
        [
            (r"/", Handler, {"handlers": handlers}),
        ]
    )
    http_server = tornado.httpserver.HTTPServer(application)
    http_server.listen(8080)

    try:
        await asyncio.sleep(20.0)
    finally:
        http_server.stop()
        await asyncio.gather(*(handler.aclose() for handler in handlers))


asyncio.run(my_function())

@bdarnell bdarnell changed the title websocket: task destroyed but was pending, _even after closing connections_. websocket: Async version of WebSocketHandler.close is needed May 4, 2023
@bdarnell
Copy link
Member

bdarnell commented May 4, 2023

The root cause of this issue is that WebSocketHandler.close is not immediate - it begins the process of shutting down the websocket connection, but this takes a little time (the server sends a close frame to the client, then waits for the client to respond with its own close frame before closing the TCP connection). We don't currently have a clean way to wait for this process to complete - @graingert 's use of the on_close hook is the best workaround for now. We should incorporate something like that into WebSocketHandler to give you a good way to close all your connections and wait for them to shut down cleanly.

bdarnell added a commit to bdarnell/tornado that referenced this issue May 15, 2023
Per the warning in the asyncio documentation, we need to hold a strong
reference to all asyncio Tasks to prevent premature GC. Following
discussions in cpython (python/cpython#91887),
we hold these references on the IOLoop instance to ensure that they are
strongly held but do not cause leaks if the event loop itself is
discarded.

This is expected to fix all of the various "task was destroyed but
it is pending" warnings that have been reported. The
IOLoop._pending_tasks set is expected to become obsolete if
corresponding changes are made to asyncio in Python 3.13.

Fixes tornadoweb#3209
Fixes tornadoweb#3047
Fixes tornadoweb#2763

Some issues involve this warning as their most visible symptom,
but have an underlying cause that should still be addressed.
Updates tornadoweb#2914
Updates tornadoweb#2356
jack-hegman pushed a commit to jack-hegman/tornado that referenced this issue Aug 16, 2023
Per the warning in the asyncio documentation, we need to hold a strong
reference to all asyncio Tasks to prevent premature GC. Following
discussions in cpython (python/cpython#91887),
we hold these references on the IOLoop instance to ensure that they are
strongly held but do not cause leaks if the event loop itself is
discarded.

This is expected to fix all of the various "task was destroyed but
it is pending" warnings that have been reported. The
IOLoop._pending_tasks set is expected to become obsolete if
corresponding changes are made to asyncio in Python 3.13.

Fixes tornadoweb#3209
Fixes tornadoweb#3047
Fixes tornadoweb#2763

Some issues involve this warning as their most visible symptom,
but have an underlying cause that should still be addressed.
Updates tornadoweb#2914
Updates tornadoweb#2356

(cherry picked from commit bffed1a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants