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

feat: Set timeout in graphql api #2356

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: Set timeout in graphql api #2356

wants to merge 5 commits into from

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Jul 1, 2024

relation #2045
Since graphql queries can be written in various forms, queries that place a lot of load on the server may be created.
Since users using the API cannot know how each query operates, it is best to prevent queries that are too costly on the server.
So, set a timeout to prevent queries that take too long.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue

📚 Documentation preview 📚: https://sorna--2356.org.readthedocs.build/en/2356/


📚 Documentation preview 📚: https://sorna-ko--2356.org.readthedocs.build/ko/2356/

Copy link

graphite-app bot commented Jul 1, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added area:docs Documentations comp:manager Related to Manager component size:M 30~100 LoC labels Jul 1, 2024
GQLMutationUnfrozenRequiredMiddleware(),
GQLMutationPrivilegeCheckMiddleware(),
],
result = await asyncio.wait_for(
Copy link
Member

@achimnol achimnol Jul 6, 2024

Choose a reason for hiding this comment

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

Now async with asyncio.timeout(): ... is the recommended timeout pattern.
https://docs.python.org/3/library/asyncio-task.html#timeouts
Also, asyncio.wait_for() raises TimeoutError instead of asyncio.TimeoutError as of Python 3.11. Have you checked the timeout handler actually catches the timeout by injecting an arbitrary sleep?

result = await _handle_gql_common(request, params)
return web.json_response(result.formatted, status=200)
except asyncio.TimeoutError:
return web.json_response(status=504)
Copy link
Member

Choose a reason for hiding this comment

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

We should have a structured error output following ai.backend.manager.api.exceptions module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:manager Related to Manager component size:M 30~100 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants