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.

Sign up for GitHub

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

Jump to bottom

Bug: Router context not being available when using AppSync with async resolvers #5290

Open
bjorger opened this issue Oct 2, 2024 · 3 comments · May be fixed by #5317
Open

Bug: Router context not being available when using AppSync with async resolvers #5290

bjorger opened this issue Oct 2, 2024 · 3 comments · May be fixed by #5317
Assignees
@leandrodamascena
Labels
bug Something isn't working help wanted Could use a second pair of eyes/hands

Comments

@bjorger
Copy link

bjorger commented Oct 2, 2024 •
edited
Loading

Expected Behaviour

When accessing the router.context in a resolver, with async resolvers, it should not be an empty dict, but much rather expose the context injected with app.append_context

Current Behaviour

When using router.context in async resolvers, the router.context is an empty dict

Code snippet

def lambda_handler(event, context):
    token = extract_auth_token_from_event(event)
    logger.info("token", extra={"token": token})
    app.append_context(
        isAdmin=True,
    )
    result = app.resolve(event, context)

    return asyncio.run(result)

@router.resolver(type_name="Mutation", field_name="addPet")
async def addPet(pet: dict):
    logger.info("Handling add pet mutation")
    logger.info("Context", extra={"resolver_context": router.context}) // this shows that router.context is an empty dict.
    
    return { "name": "Peter", "type": "Cat" }

Possible Solution

No response

Steps to Reproduce

Just plug in the lambda_handler and mutation I attached above, deploy it and run the mutation via the AppSync console

Powertools for AWS Lambda (Python) version

latest

AWS Lambda function runtime

3.8

Packaging format used

Lambda Layers

Debugging logs

No response
@bjorger bjorger added bug Something isn't working triage Pending triage from maintainers labels Oct 2, 2024
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Oct 2, 2024

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@bjorger bjorger changed the title Bug: Router context now being available when using AppSync with async lambda Bug: Router context not being available when using AppSync with async lambda Oct 2, 2024
@bjorger bjorger changed the title Bug: Router context not being available when using AppSync with async lambda Bug: Router context not being available when using AppSync with async resolvers Oct 2, 2024
@leandrodamascena
Copy link
Contributor

Hey @bjorger! Thanks for opening this bug. I was able to reproduce this error and I can confirm this is a bug.

The problem is because we need to clean the context after the execution to prevent it from remaining in memory on the next run, but it looks like we have a problem with coroutines because it is being cleaned up before the coroutine runs.

I'm adding this to the backlog to investigate more this or next week. If you have time and found a solution before me, I'd love you submit a PR to fix this.

@leandrodamascena leandrodamascena added help wanted Could use a second pair of eyes/hands and removed triage Pending triage from maintainers labels Oct 2, 2024
@leandrodamascena
Copy link
Contributor

Hi @bjorger! I spent some time today investigating this issue and I don't know if we can fix it without a refactoring that will be a breaking change for our customers. The problem here is because we have no control over the evenloop or tasks that will be scheduled by asyncio and we clean up the context before the coroutine starts. I would try to start a new eventloop to control it, but I have no idea about the consequences and I don't know if that works.

To be honest I think this is an error in our implementation, we should have control over the async execution and execute it inside the AppSyncResolver and not the customer doing that. If you see our async_batch_resolver function we do what I said and we don't have this bug.

But I have an alternative for this. Not the best developer experience, but it solves the problem and we can refactor this in v4.

1 - I will check if the function resolver is a coroutine here and if so, I will not call self.clear_context(). Then we can preserve the context.
2 - This solution creates a side effect that the context can persist across multiple invocations, but I don't see this breaking customers today because this is not working anyway in async invocation.
3 - To solve the problem in the item 2, if the customer does not want to preserve the context across multiple invocations (there are cases where customers can preserve it), the customer should invoke self.contex.clear() after the resolver is executed.
4 - I will update the documentation to make it clear for customers.

I'm still open to hearing other opinions/ideas to fix this, but if we don't have any, what do you think of this solution?

Thanks

@leandrodamascena leandrodamascena linked a pull request Oct 5, 2024 that will close this issue
Open
7 tasks
@leandrodamascena leandrodamascena linked a pull request Oct 5, 2024 that will close this issue
fix(appsync): make contextual data accessible for async functions #5317
Open
7 tasks
@dreamorosi dreamorosi mentioned this issue Oct 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@leandrodamascena leandrodamascena

Labels
bug Something isn't working help wanted Could use a second pair of eyes/hands
Projects
Powertools for AWS Lambda (Python)
Status: Working on it
Milestone
No milestone
2 participants
@leandrodamascena @bjorger