collect_subfields memoization keyed on id() of FieldDetails that are not kept alive returns another field's subfields #284

In 3.3.0, Executor.collect_subfields memoizes per execution in self._relevant_sub_fields, keyed on the return type and the id() of each FieldDetails in the list:

key = (
    (return_type, id(field_details_list[0]))
    if len(field_details_list) == 1
    else (return_type, *map(id, field_details_list))
)

The cache keeps the collected subfields as values, but nothing keeps the FieldDetails of the key alive. Once a field has finished, its FieldDetails (a 3-tuple) can be freed. CPython then often reuses that address for the next tuple of the same size, and if that tuple is the FieldDetails of another field returning the same object type, the lookup hits the stale entry: the second field is answered with the first field's selection set.

We saw it intermittently (three times in a few hundred full runs) with queries like:

{
  items { ... on ItemPage { edges { node { key kind } } } }
  item(id: "…") { name }
}

where every edges.node came back with only name, the selection of item. A client can thus receive fields it did not ask for and miss the ones it did.

Deterministic reproduction on CPython (freeing one FieldDetails and building the next, which CPython's allocator places at the same address; a gc.collect() in between allocates and hides it):

from graphql import GraphQLField, GraphQLObjectType, GraphQLSchema, GraphQLString, parse
from graphql.execution.collect_fields import FieldDetails
from graphql.execution.executor import Executor

item = GraphQLObjectType("Item", {"key": GraphQLField(GraphQLString), "name": GraphQLField(GraphQLString)})
schema = GraphQLSchema(GraphQLObjectType("Query", {"a": GraphQLField(item), "b": GraphQLField(item)}))
document = parse("{ a { key } b { name } }")
executor = Executor.build(schema, document)
a_node, b_node = document.definitions[0].selection_set.selections

first = [FieldDetails(a_node, None)]
executor.collect_subfields(item, first)
address = id(first[0])
del first
second = [FieldDetails(b_node, None)]

assert id(second[0]) == address  # usually true on CPython
collected = executor.collect_subfields(item, second)
print(list(collected.grouped_field_set))  # ['key'] — expected ['name']

This looks related to #91. The comment above the key notes that list ids may be reused, but the same holds for the ids of the items once they are freed.

Possible fixes: keep the key's FieldDetails alive with the cached value (for example store (field_details_list, collected_fields) and compare by identity on hit), or key on the FieldNodes (id(fd.node)), which live as long as the document, plus whatever else of FieldDetails affects the collection (defer_usage, fragment_variable_values).

Our workaround meanwhile is an execution context whose collect_subfields keeps every field_details_list it is given referenced until the operation ends.