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

Allow directly instantiating the Scheduler class #69

Closed
gjcarneiro opened this issue Oct 25, 2018 · 9 comments · Fixed by #353
Closed

Allow directly instantiating the Scheduler class #69

gjcarneiro opened this issue Oct 25, 2018 · 9 comments · Fixed by #353

Comments

@gjcarneiro
Copy link

Documentation says:

User should never instantiate the class but call create_scheduler() async function.

However I don't understand why this restriction. In the age of type checking, it makes usage of this scheduler weird, because you cannot have await in another class' __init__, which means that you have to defer creating a Scheduler object as a member of an object until later.

For example:

class MyWorker:

    def __init__(self):
        # this doesn't work:
        # self.scheduler: aiojobs.Scheduler = await aiojobs.create_scheduler()
        # so instead I have to do this
        self.scheduler: Optional[aiojobs.Scheduler] = None

    async def start(self):
        self.scheduler = await aiojobs.create_scheduler()

    async def close(self):
        await self.scheduler.close()

    async def do_something(self):
         ....
         assert self.scheduler is not None  # otherwise mypy will complain that scheduler can be None
         await self.scheduler.spawn(something)

Instead I would like to be able to instantiate Scheduler directly, perhaps add a start() async method to it to allow for any future async initialisation that might be needed:

class MyWorker:

    def __init__(self):
        self.scheduler = aiojobs.Scheduler()

    async def start(self):
        await self.scheduler.start()

    async def close(self):
        await self.scheduler.close()

    async def do_something(self):
         # no need to check for None!
         await self.scheduler.spawn(something)

To make this work, all we would need would be a simple PR that:

  1. add a async def start(): pass method to Scheduler;
  2. change the documentation accordingly.

Does that sound acceptable?

@asvetlov
Copy link
Member

I'm not ready to make the decision now.

Sorry, I'm overwhelmed with adding strong type annotations to aiohttp.
Until it is done no other activities from me in OSS libraries most likely.

@gjcarneiro
Copy link
Author

Strong type annotations in aiohttp sounds excellent, keep up the good work :)

It's ok, I'm in no big hurry.

@momocow
Copy link

momocow commented Jan 14, 2020

I have the same question too.

After digging a little bit into the codes, I'm wondering why create_scheduler should be declared async. It seems that a Scheduler instance is returned immediately.

async def create_scheduler(*, close_timeout=0.1, limit=100,
pending_limit=10000, exception_handler=None):
if exception_handler is not None and not callable(exception_handler):
raise TypeError('A callable object or None is expected, '
'got {!r}'.format(exception_handler))
loop = asyncio.get_event_loop()
return Scheduler(loop=loop, close_timeout=close_timeout,
limit=limit, pending_limit=pending_limit,
exception_handler=exception_handler)

@asvetlov
Copy link
Member

Well, technically the scheduler can be instantiated directly in constructor but it should have a check for running event loop like here: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L206
Without the check the change is error-prone.

@momocow
Copy link

momocow commented Jan 14, 2020

@asvetlov Thanks for the response.

However, I've tried but still not fully understand why it's necessary to be async to perform checks against the running loop.

The link you provided is inside a constructor which is also a synchronized behavior.

Can you explain a bit more about this?

Maybe provide an example for me to show what is actually checked against the loop and the check is performed in async way.

Many thanks!

@asvetlov
Copy link
Member

@momocow
Copy link

momocow commented Jan 15, 2020

Oh, I got it. It's all about to ensure a running loop when initiating a scheduler.

Since create_scheduler is defined as a coroutine via async def, we have to ensure a loop is running so that we can await on it to get the returned scheduler.

Thank you very much for the link and the nice work on asyncio, @asvetlov.

@asvetlov
Copy link
Member

Welcome!
As I said above, there is a possibility to make all required checks in Scheduler constructor and deprecate create_scheduler() usage slowly but it requires more work. I have no capacity to do it in the near future.

@Dreamsorcerer
Copy link
Member

I suspect we could make this change now. The loop is no longer passed explicitly, and the asyncio.create_task() call will error in the constructor if there is no running loop: https://github.com/aio-libs/aiojobs/blob/master/aiojobs/_scheduler.py#L14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants