Potential memory leak

[ediskandarov]

fastapi-events/fastapi_events/middleware.py

Lines 23 to 28 in a5a2823

	token: Token = event_store.set(deque())
	try:
	await self.app(scope, receive, send)
	finally:
	await self._process_events()
	event_store.reset(token)

        token: Token = event_store.set(deque())
        try:
            await self.app(scope, receive, send)
        finally:
            await self._process_events()
        event_store.reset(token)

If await self._process_events() raises an exception then event_store.reset(token) does not happen.

[ediskandarov]

contents of this may be useful

https://stackoverflow.com/questions/67062025/will-a-contextvar-leak-memory-in-async-logic-if-not-reset-after-exception

[melvinkcx]

Thanks for bringing this to my attention!

Went through the link you provided above, and one of the references:
https://stackoverflow.com/questions/66856654/what-happens-if-i-dont-reset-pythons-contextvars

It doesn't seem like a memory leak can occur even if self._process_events() raises an Exception after your PR #6. The token is now declared as a local variable within the scope of __call__(). It won't and can't be referenced after the execution of __call__(). -> Hence, will be GC'd

[melvinkcx]

Your PR #8 will change the intended behaviour of the event processing. If both await self.app(scope, receive, send) and await self._process_events() are wrapped within the same try block, any exceptions raised during the request processing will also mean no events will be handled.

[ediskandarov]

perhaps, a nested try block would be a safe approach here?

[melvinkcx]

Though, I'm really curious to perform some sort of tests to verify both of our hypothesis

[ediskandarov]

it should either rely on the garbage collector and does not care about context var reset at all, or come up with a robust context var management technique

[melvinkcx]

Yes, a nested try...finally block could be the solution.

[melvinkcx]

I agree with you, since we're using .reset() here, we should do it properly instead of relying on GC to clean it up in case of any exceptions

[ediskandarov]

here's an example inspired by loguru's logger

    @contextlib.contextmanager
    def contextualize():
        token = context.set(deque())

        try:
            yield
        finally:
            context.reset(token)

and then use this context manager in the middleware

[melvinkcx]

I like this solution, we can potentially do:

with event_store_ctx():
    try:
         await self.app(scope, receive, send)
    finally:
         await self._process_events()

[ediskandarov]

yeah, something like that

[melvinkcx]

Feel free to use a context manager. If not, we can stick to the nested try block for now.
I can implement the context manager afterwards =)

Again, thanks for your contribution!

[ediskandarov]

I've updated pull request