Skyscanner / aiotask-context

Contextual information for asyncio tasks
MIT License
161 stars 25 forks source link

Allow arbitrary task context edition #18

Closed jsoucheiron closed 5 years ago

jsoucheiron commented 7 years ago

Add the option to define the task when getting/setting keys from context

pfreixes commented 7 years ago

Do you have a rationale and examples about when its necessary access to the task context from other tasks?

jsoucheiron commented 7 years ago

In our case we have a decorator that converts a coroutine into a task. We want this decorator to add content to the context from the newly created task.

def convert_to_task_and_add_variable(func):
    @functools.wraps(func)
    async def wrapper(self, *args, **kwargs):
        task = asyncio.get_event_loop().create_task(func(self, *args, **kwargs))
        task.context["my_variable"] = self.variable
        return await task

    return wrapper

It's kind of ugly, and I'd rather:

aiotask_context.set('my_variable', self.variable, task=task)
argaen commented 7 years ago

@pfreixes I'm okay with it for two reasons:

EDIT: It's True the task.context['my_variable'] is simple enough to do this but its coupled to the current attribute being called context and this context being a mapping. However I don't have a strong opinion on this.

pfreixes commented 7 years ago

This change IMHO goes into a different direction of the package direction. The idea of the package was implement a way to set up a local context for asyncio tasks and with a minimum interface to get and set values from this context following the Thread locals implementation. Indeed this is the main goal of the PEP 0550 [1].

Having a way to modify the context from other tasks raises other issues that might end up with race conditions u others. And this IMHO should be implemented by the user using tools that are ready for that.

If the main requirement is given a way to set up the context of the task without polluting the context of the current task, the PR would need to focus on giving an interface that provides exactly this feature.

Take into account that with the current implementation there is no guarantee that the task won't be scheduled before the context is modified. Perhaps:


async def child():
    print(aiotask.get('foo'))

async def parent():
    task = asyncio.ensure_future(child())
    await asyncio.sleep(0)
    aiotask.set('foo', 1, task)

This by design won't be able to set up the context for the children task because will be scheduled before because of the IO in the parent task. This is IMHO a prone error pattern and I would prefer to have something that behaves deterministically to set up the context before this task is started.

thoughts?

[1] https://www.python.org/dev/peps/pep-0550/

jsoucheiron commented 7 years ago

That's a fair point. I really don't have a better solution though.

I was trying to avoid the necessity of the user accessing the context property directly, as it's not part of the API.

As accessing the context directly is a reasonable workaround, I'm on no hurry to get this fixed soon. But there are definitely some cases where you'll want to edit task contexts from outside and we should make sure there's a reasonable API for that.

argaen commented 7 years ago

Yup, @pfreixes raises a good point regarding race conditions and this being error prone.

I suggest we close the MR then and as an action try to collect a list of those cases you mention @jsoucheiron and see how we could solve this in the simplest way.