Race condition causes resolver method injections to be undefined

[theseyi]

Describe the Bug
TypeGraphQL has been fantastic so far, however, using serverless-offline or serverless in general with TypeGraphQL is known to have various limitations and workarounds have been prescribed for some of these issues

This is a bug that happens when using a workaround to clear the global metadata storage on every incoming query when running serverless-offline before any imports: global.TypeGraphQLMetadataStorage = undefined.

This bug presents itself when there are two concurrent queries / requests being handled asynchronously, query A and query B and they happen to use the same metadata types.

Sequence:

1. query A comes in and get's a brand new global.TypeGraphQLMetadataStorage (storeA)
2. Decorators for resolvers get parsed and populated into storeA including FieldResolver decorators and Args()
3. Some non-blocking async work begins to process and resolve query A
4. query B comes in and get's a brand new global.TypeGraphQLMetadataStorage (storeB)
5. Note that store A is no longer reachable, since the global reference will be set to undefined and replaced with store B
6. Decorators for resolvers get parsed and populated into store B, with their own runtime instances
7. query B completes and resolves fine since the store was built with it's own type instances
8. query A subsequently asynchronously resumes, but now requests to the store, store B, will be invalid because the instances no longer pass referential equality checks. For example convertArgsToInstance which calls getArgsType that does a strict equals comparison to find the matching target in argumentTypes on the global store.

The problem is that shared global state is mutable and therefore presents invalid values to callers when they are processed out of order.

Expected Behavior
That TypeGraphQL is able to inject the correct params for query resolvers
Shared immutable global state, but this isn't quite feasible with serverless architectures given how TypeGraphQL is implemented, and leads to the other problems linked above
Or, more expensive and wasteful, each request receives its own private TypeGraphQLMetadataStorage

Logs

{
    "errors": [
        {
            "message": "Cannot read property 'fields' of undefined",
            "locations": [
                {
                    "line": 15,
                    "column": 7
                }
            ],
            "path": ["..."],
            "extensions": {
                "code": "INTERNAL_SERVER_ERROR",
                "exception": {
                    "stacktrace": [
                        "TypeError: Cannot read property 'fields' of undefined", "..."
                     ]
                }
            }
        }
    ],
    "data": {
        "viewer": null
    }
}

Environment (please complete the following information):

• OS: macOS
• Node 12
• Package version 1.1.1
• TypeScript version 4.2.3

Additional Context
Add any other context about the problem here.

[MichalLytek]

this is a bug that happens when using a workaround to clear the global metadata storage on every incoming query when running serverless-offline before any imports: global.TypeGraphQLMetadataStorage = undefined.

Modifying not exposed and not documented private parts is always done on your own responsibility.
You can't report a bug if you mess up with the internals.
So sorry, but I can't fix it 🔒

when running serverless-offline

Why do you need that? We use serverless with TypeGraphQL and even subscriptions without any issues 🤔

[theseyi]

Modifying not exposed and not documented private parts is always done on your own responsibility.
You can't report a bug if you mess up with the internals.
So sorry, but I can't fix it 🔒

@MichalLytek this does not quite modify any internals of TypeGraphQL. There are only two things being done and one of them was recommended by you

1. Using this in a serverless environment (e.g. apollo-lambda-server) #96 (comment)
2. The other is actually similar to what the community found as a solution to another issue Added basic implementation for #124 #163 #issuecomment-513566403 which is to delete the cache so each query get's a clean slate

Why do you need that? We use serverless with TypeGraphQL and even subscriptions without any issues 🤔

Running into similar issues those mentioned in the links above. You may not be running into the same issue, as mentioned, this happens when there are two concurrent queries that use the same schema metadata types and one resolves before the other. If this does not happen then you probably won't encounter this

[MichalLytek]

But why do you clear the metadata storage? It's prohibited to use that on your own, it's not a part of a public API and it's never mentioned in the docs. It's exposed only for my internal usage in the tests.

one of them was recommended by you

You should have a single schema build once, stored in the global property in order to avoid timely and costly building the schema on every lambda trigger.

I never suggested anyone cleaning the metadata storage. It's against the TypeGraphQL architecture and can be changed even in a patch release.

[theseyi]

@MichalLytek It's kind of the reverse of the same issue, I will try to break it down to the best of my knowledge.

To ensure you have a single store with serverless-offline, you recommend:

(global as any).schema = (global as any).schema || await buildSchema(/*... */);
const schema = (global as any).schema;

However, serverless-offline on every query / request re-evaluates the decorators in the source, and each decorator, for example, @FieldResolver(), invokes collectFieldResolverMetadata

type-graphql/src/decorators/FieldResolver.ts

Line 45 in 3733b1e

getMetadataStorage().collectFieldResolverMetadata({

So the decorator does a push into the related store cache. (So each query will push the same metadata but for it's own invocation into the store)

However, buildResolversMetadata

type-graphql/src/metadata/metadata-storage.ts

Line 201 in 3733b1e

private buildResolversMetadata(definitions: BaseResolverMetadata[]) {

is subsequently invoked. What this does is iterate through the values currently in the cache and mutate them such that if the current target is not referentially equal to the value in the cache, it will remove it through this filter operation:

type-graphql/src/metadata/metadata-storage.ts

Line 208 in 3733b1e

param => param.target === def.target && def.methodName === param.methodName,

Hope that sounds fair so far

The import of this, is as mentioned previously, if a query A happens to resolve after this mutation has occurred on the global cache for queryB, then it's metadata is no longer present in the cache and you get issues like undefined injections, when the field resolver is now ready to handle the query. This is because of the previous filter.

I want to emphasize that this happens when a client makes two concurrent queries that depend the same metadata types (e.g. @Args() decorators), that resolve out of order. If you have one query at a time, you won't run into this issue. Probably also mutations do not have this issue since they are always handled serially.

Hope that explains the issue?

[MichalLytek]

TypeGraphQL was never designed to support paralel schema building, mostly because of the BuildContext global singleton workaround.

You can't just expect me to fix that because it's not a bug - it doesn't happen until you break the internals by clearing the metadata storage.

You need to figure out a way to not reevaluate the decorators by serverless-offline.

[theseyi]

Thanks @MichalLytek I was more looking for your input on addressing it, and hopefully contributing a PR to resolve it here or in serverless-offline as appropriate, maybe I shouldn't have called it a bug 😓

But the issue in my last comment happens without clearing the metadata storage, this happens when using the solution you recommended to retain the same metadata storage, and if there are concurrent queries that resolve out of order.

(global as any).schema = (global as any).schema || await buildSchema(/*... */);
const schema = (global as any).schema;

That's why the 2nd "fix" was then attempted, so without that the 1st issue: ("filtering out targets that are not current"), still presents itself.

I guess what you're saying is that all queries should be done in a way that they don't happen concurrently using the same metadata

[theseyi]

It seems using --allowCache with serverless-offline 6.* is a better replication of conditions in an actual Lambda, handler code does not get re-evaluated on each request in this case

[theseyi]

Why do you need that? We use serverless with TypeGraphQL and even subscriptions without any issues 🤔

Is there a chance there is some public documentation around this for serverless-offline?

[MichalLytek]

@theseyi
Put your working solution in the mentioned issue, so others can find that and check if it works for them.
The docs are already long, I can't put there info about integration with every other libs or frameworks.

[theseyi]

Put your working solution in the mentioned issue, so others can find that and check if it works for them.
The docs are already long, I can't put there info about integration with every other libs or frameworks.

Sounds good I can comment here, or does creating a PR to add it to the FAQ work?