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

feature request: singleton for coroutine functions #114

Closed
zerlok opened this issue Nov 5, 2024 · 13 comments · Fixed by #129
Closed

feature request: singleton for coroutine functions #114

zerlok opened this issue Nov 5, 2024 · 13 comments · Fixed by #129
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@zerlok
Copy link
Contributor

zerlok commented Nov 5, 2024

Singleton provider expects only synchronous callable object. I think it would be great to support coroutine functions too.

At the moment the following code raises not awaited warning & obj1 is not MyObj instance.

import asyncio

from that_depends import BaseContainer, providers


class MyObj:
    pass


async def create_my_async_object() -> MyObj:
    await asyncio.sleep(0.0001)
    return MyObj()


class DIContainer(BaseContainer):
    async_obj = providers.Singleton(create_my_async_object)


async def main() -> None:
    obj1 = await DIContainer.async_obj()
    obj2 = await DIContainer.async_obj()

    assert obj1 is obj2
    assert isinstance(obj1, MyObj)


if __name__ == "__main__":
    asyncio.run(main())

Output:

...
    assert isinstance(obj1, MyObj)
           ^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
sys:1: RuntimeWarning: coroutine 'create_my_async_object' was never awaited
@lesnik512
Copy link
Member

Resource can be used and async generator instead of coroutine

@alexanderlazarev0 alexanderlazarev0 added the enhancement New feature or request label Nov 9, 2024
@zerlok
Copy link
Contributor Author

zerlok commented Nov 10, 2024

@lesnik512 , if client has coroutine function in other module and wants to use a result as singleton - he needs to add extra code to put it into Resource and then use as singleton:

from my_app.my_module import create_my_obj_async, MyObj

async def _init_my_obj(arg1: str, arg2: int, ...) -> AsyncIterator[MyObj]:
    yield await create_my_obj_async(arg1, arg2, ...)

class DIContainer(BaseContainer):
    async_obj = providers.Resource(_init_my_obj)

In my opinion, Singleton can handle async coroutine and client won't need to write extra code.

@lesnik512
Copy link
Member

lesnik512 commented Nov 10, 2024

There are two options I can think of:

  1. Add AsyncSingleton which accepts coroutines. Seems like a cleaner way
  2. Allow Singleton to accept coroutines. Can lead to problems with type checking, because typing.Callable[..., T_Co] conflicts with typing.Callable[..., typing.Awaitable[T_Co]]

@zerlok
Copy link
Contributor Author

zerlok commented Nov 10, 2024

@lesnik512

I'm ok with 1 option.

Just for note, in the second we can first check for coroutine function Callable[..., Awaitable[T_co]] and use awaits during resolving. If it's not coroutine function then it's a regular function Callable[..., T_co] - use regular calls without awaits. Type Callable[P, V_co] | Callable[P, Coroutine[Any, Any, V_co]] has no conflicts from my perspective.

@lesnik512
Copy link
Member

Type Callable[P, V_co] | Callable[P, Coroutine[Any, Any, V_co]] has no conflicts from my perspective.

as a matter of fact it does. At least, I haven't found a way to work it together in one union type.

Before we start to implement this can you please provide a real-life example of when an async singleton might be useful?

@lesnik512
Copy link
Member

I believe it is important to only add new features if they are truly necessary and do not introduce unnecessary complexity or hacks into the codebase.

@zerlok
Copy link
Contributor Author

zerlok commented Nov 10, 2024

I can share a part of code, so here is async contstructor for a sender

class MySender:
    @classmethod
    async def create(cls, manager, config) -> MySender:
        factory = manager.get_sender_factory()
        
        return cls(
            sender=await factory.create_sender(...)
        )

    def __init__(self, sender):
        self.__sender = sender

    async def send(self, ...):
        await self.__sender.send(...)

The idea is to use a single instance of that publisher on demand in the application. For now it's not possible, so AsyncFactory is used as provider.

class DIContainer:
    # ...
    my_sender = providers.AsyncFactory(MySender.create, ...)

I pretty sure that anything that can be put into Factory provider, can be also put into Singleton provider .

Also, the idea of the library is to support async by default.

@lesnik512
Copy link
Member

Don't you need to tear down sender at the end? Seems like sender is needed to be moved to Resource

@lesnik512
Copy link
Member

And why is sender created asynchronously? What's happening inside create_sender?

@zerlok
Copy link
Contributor Author

zerlok commented Nov 11, 2024

Sender uses aio pika channel and requires initialized channel for work. Channel initialization is done via await channel / await channel.initialize() . That's why it is need to create it asynchronously. The created channel is destroyed on amqp manager tear down. Amqp manager instance is provided via Resource.

@lesnik512
Copy link
Member

@zerlok Can you, please share the code of create_sender?

@zerlok
Copy link
Contributor Author

zerlok commented Nov 12, 2024

somthing like this

async def create_sender(
    self,
    # ...
) -> Sender:
    factory = PublisherFactory(
        channel_factory=self.__channel_factory,
        # params ...
    )

    publisher = await factory.create_publisher(...)

    return Sender(
        publisher=publisher,
        # ...
    )


async def create_publisher(
    self,
    # ...
) -> Publisher:
    channel = await self.__channel_factory.create_channel(...)
    
    exchange = await channel.declare_exchange(...)

    return Publisher(
        connection=self.__connection,
        exchange=exchange,
        # ...
    )


async def create_channel(
    self,
    # ...
) -> aio_pika.Channel:
    impl = self.__get_connection()
    assert isinstance(impl, aio_pika.Connection)

    channel = await impl.channel(...)
    assert isinstance(channel, aio_pika.Channel)

    # ...

    return channel

@lesnik512
Copy link
Member

@zerlok I agree, could be useful.

@lesnik512 lesnik512 added the good first issue Good for newcomers label Nov 19, 2024
@lesnik512 lesnik512 linked a pull request Nov 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants