Memory leak in create_temporary_request_context() method

[dangusev]

Hi @ashleysommer , it seems that there is a memory leak in the handling of the websocket requests. I am able to reproduce it with https://github.com/ashleysommer/sanic-cors

Long story short:

There's a call of SanicPluginsFramework.create_temporary_request_context in method SanicPluginsFramework._run_request_middleware which preserves a link to the current request in shared context. It seems that this temporary context is intended to be deleted in SanicPluginsFramework.delete_temporary_request_context, but it doesn't happen because method SanicPluginsFramework._run_response_middleware won't be called in case of websockets (response is None and response middleware won't be called as well https://github.com/huge-success/sanic/blob/master/sanic/app.py#L950).

How to reproduce:

0. Download issue.zip
1. Install sanic, sanic_cors, tqdm, aiohttp
2. Run ws_server.py (in attached zip)
3. Run ws_bench.py
4. Look at chain.png in the same directory, there will be a reference graph with stalled reference to random CIMultiDict instance
5. Commenting out SanicPluginsFramework.create_temporary_request_context fixes the issue

I briefly looked at the source code but didn't come up with any guess how to fix it. Any ideas?

Thanks a lot!

[ashleysommer]

@dangusev Thank you for filing this bug! You're absolutely spot on with your diagnosis. I don't personally use websockets, so I haven't seen this happen before now.

I think the easiest way forward is to just disable the temporary request context creation, if the request is a websocket request. Get that fix out the door, then look at making a better fix after that.

[dangusev]

Well, it doesn't seem that easy) For now I don't see any proper way to determine whether the request is websocket or not.

The one I came up with looks like this and it's a bit dirty:

from sanic.websocket import WebSocketProtocol

def is_websocket_request(request):
    try:
        protocol = request.transport.get_protocol()
    except AttributeError:
        protocol = request.transport._protocol
    return isinstance(protocol, WebSocketProtocol)

Probably you know better way?

[ashleysommer]

@dangusev, no I don't know of any other way to detect websocket connections. I might approach this differently.

[ashleysommer]

Looks like this affects any case where Response is None, not just Websockets, so checking if a request is a websocket request is probably not the right option.

This is my plan: 1) In SPF, introduce a new type of Middleware, called "cleanup" middleware that is always called after response middleware, regardless of whether a response is None, or if there was an error. 2) In SPF, wrap app's default handler, to add the new cleanup middleware runner after running the default handler.

[ashleysommer]

@dangusev I've pushed a fix, v0.7.0. It seems to me that it fixes the problem, are you able to please test and verify that it has fixed what you were seeing?

[dangusev]

Yep, memory consumption is constant now, tested with 10k reconnects. Thanks a lot for such a quick fix! Could you please bump spf version for plugins like https://github.com/ashleysommer/sanic-cors as well?

[ashleysommer]

Thanks for the reminder. Yes, I will.