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

Fix: DOS-625 Address action execution blocking new requests #189

Merged

Conversation

sam-causalens
Copy link
Contributor

Motivation and Context

  • An internal project was stalling requests when multiple actions were in flight that led to the app appearing broken for a time until the first action completed.

Implementation Description

  • The usage of BackgroundTasks in the new execute_action code was found to be causing blockages of new requests being picked up by the event loop. I think this maybe a reccurance of Background tasks don't work with middleware that subclasses BaseHTTPMiddleware encode/starlette#919 which they claim is resolved, but still doesn't appear to be working for us. It may be due to one of our middlewares behaving in a particular way that causes this, but the fix applied here appears to be the simplest fix for now.
  • In this instance we have replaced the usage of background_tasks.add_task with asyncio.create_task that will run the task in the loop without worrying about its result.

Any new dependencies Introduced

No

How Has This Been Tested?

Should be covered by existing unit tests

PR Checklist:

  • I have implemented all requirements? (see JIRA, project documentation).
  • I am not affecting someone else's work, If I am, they are included as a reviewer.
  • I have added relevant tests (unit, integration or regression).
  • I have added comments to all the bits that are hard to follow.
  • I have added/updated Documentation.
  • I have updated the appropriate changelog with a line for my changes.

Screenshots (if appropriate):

Reproduction Code (in a dara app):

@action
async def test_action(ctx):
    print(f'running action {ctx}')
    for i in range(20):
        await anyio.sleep(0.2)
    print(f'finished action {ctx}')

config.add_page('action bug', Stack(*[Button(f'action-{x}', onclick=test_action(x)) for x in range(10)]))

Behaviour Before:
https://github.com/causalens/dara/assets/58470795/8ac1fef4-4505-4a39-a911-30fe8f794b7d

Behaviour After:
https://github.com/causalens/dara/assets/58470795/bd65f4b2-dc28-4669-8bbd-5b0827fe4f62

* The usage of BackgroundTasks in the new execute_action code was found to be causing blockages of new requests being picked up by the event loop. I think this maybe a reccurance of encode/starlette#919 which they claim is resolved, but still doesn't appear to be working for us. It may be due to one of our middlewares behaving in a particular way that causes this, but the fix applied here appears to be the simplest fix for now.
* In this instance we have replaced the usage of background_tasks.add_task with asyncio.create_task that will run the task in the loop without worrying about its result.
Copy link
Collaborator

@krzysztof-causalens krzysztof-causalens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@krzysztof-causalens krzysztof-causalens merged commit 21bd0ad into master Feb 6, 2024
2 checks passed
@krzysztof-causalens krzysztof-causalens deleted the dos-625-resolve-actions-blocking-webserver branch February 6, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants