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

Graphql query resolution gets very slow with middlewares. #708

Closed
gghildyal opened this issue Oct 23, 2021 · 9 comments · Fixed by mirumee/ariadne-website#88
Closed

Graphql query resolution gets very slow with middlewares. #708

gghildyal opened this issue Oct 23, 2021 · 9 comments · Fixed by mirumee/ariadne-website#88
Labels
docs Focused on documentation contents

Comments

@gghildyal
Copy link

While debugging performance issues with queries that return a long list, I found that my middleware was causing a lot of slowness. At which point I reduced my middleware to a bare-bones middleware like this

class LoginRequiredMiddleware(object):
    """A middleware that raises a consistent error for unauthenticated users."""

    @staticmethod
    async def resolve(  # type: ignore
        resolver: Resolver,
        parent: Any,
        info: GraphQLResolveInfo,
        **kwargs,
    ):
            result = resolver(parent, info, **kwargs)
            if isawaitable(result):
                result = await result
            return result

With this middleware, a nested query below takes ~27 seconds. Without it, it takes 5 seconds!

query {
  operators {
      lines {
          lineId
          servicePatterns {
            stops {
              stopId
            }
          }
      }
    }
  }

can return a long list of service patterns and stops (~10,000 items).

Am I just paying the price of that extra function call? Or because I am awaiting every resolver in the middleware?

@gghildyal
Copy link
Author

This may help me - https://pypi.org/project/graphql-utilities/0.1.0/

@gghildyal
Copy link
Author

gghildyal commented Oct 23, 2021

graphene-django has adopted the changes from graphql-utilities, decorator called run_only_once which runs the middleware once per operator - See graphql-python/graphene-django#810.

A slightly modified version of run_only_once that works well for me, although I modified it to run once per request (rather than per operator) as I needed it for checking if the user is authenticated.

def run_only_once(decorator_name):
    """
        Make sure middleware is run only once,
        this is done by setting a flag in the `context` of `ResolverInfo`

        Example:
            class AuthenticationMiddleware:
                @run_only_once(name='AuthMiddleware')
                def resolve(resolver, parent, info, **kwargs):
                    pass
        """
    from functools import wraps

    def run_only_once_wrapper(resolve_func):
        @wraps(resolve_func)
        def wrapper(resolver, parent, info: GraphQLResolveInfo, **kwargs):
            context: APIContext = info.context
            if not context.run_once_middleware.get(decorator_name, False):
                info.context.run_once_middleware[decorator_name] = True
                return resolve_func(resolver, parent, info, **kwargs)
            return resolver(parent, info, **kwargs)

        return wrapper
    return run_only_once_wrapper

@rafalp
Copy link
Contributor

rafalp commented Oct 23, 2021

Speaking from few years of experience I feel that whatever you want to in GraphQL, middlewares are the worst way to go about it.

Python utility that takes schema, function and selector, then calls function against resolver of every field matched by selector would run only on service's startup (vs on every query like how middleware classes are) and then only decorates specified fields (so there aren't bazillions of function calls), something like this:

def require_auth(field, resolver):
    if field.name in ["auth", "login"]:
        return  # return default resolvers on auth field and login mutation

    @wraps(resolver)
    def require_auth(obj, info, *args, **kwargs):
        if not has_auth(info.context):
            return None
        return resolver(obj, info, *args, **kwargs)

    return require_auth(resolver)


schema = wrap_resolvers(schema, require_auth, "Query>*,Mutation>*")

Am I just paying the price of that extra function call?

isawaitable is is costful call in Python: Performance of isawaitable

@kissgyorgy
Copy link
Contributor

We ran into the same issue, we didn't know (it's not intuitive) that graphql-core changes EVERY single resolver to your middleware, so even if you have a simple attribute or dict value lookup, the middleware will run as many times as many properties your object has. In our case, more than 20,000 extra coroutines have been created, which slowed everything down by 4-5x.

For authentication, we went down one level abstraction and wrote a Starlette middleware, so every query is authenticated only once. If you need any middleware functionality, I suggest doing the same and using Starlette middleware instead of graphql-core middlewares.

@rafalp
Copy link
Contributor

rafalp commented Nov 8, 2021

ASGI middleware is the way to go for auth. GraphQL middlewares are more often a trap than useful solution.

@rafalp rafalp added the docs Focused on documentation contents label Nov 8, 2021
@rafalp
Copy link
Contributor

rafalp commented Nov 8, 2021

I'll label this as doc. We should bring this up as a warning.

@rafalp
Copy link
Contributor

rafalp commented Nov 19, 2021

I'll add section about performance impact of middlewares to our docs. Thanks for bringing this up!

@rafalp rafalp closed this as completed Nov 19, 2021
@dacevedo12
Copy link

Can confirm, middlewares and extensions are very taxing even on simple tasks

@rafalp How should one go if the intent is to cover every single resolver for something like: #649?

@kissgyorgy
Copy link
Contributor

We measured the impact of turning simple functions into coroutines and the slowdown is 100x (😱) so I don't think you can use this "middleware every resolver" without a serious performance hit.

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

Successfully merging a pull request may close this issue.

4 participants