Cancel defer/stream execution when path is nulled from error bubbling

[benjie]

Context

Schema:

type Query {
  a: A
}
type A {
  numbers: [Int]!
  alwaysThrows: Int!
}

Query:

{
  a {
    numbers @stream
    alwaysThrows
  }
}

alwaysThrows always throws, but we process selection sets top to bottom, so there will be a subsequentPayloads entry added for a.number and then alwaysThrows will throw, which will bubble to give the result:

{
  "a": null
}

followed by subsequent payloads referencing ["a", "numbers", N]. In the case that alwaysThrows throws, there should be no subsequent payloads from a.number since there's nowhere for them to go (the parent was nulled-out).

Decision

Discussed at the 2022-09-01 WG meeting. Decided that servers must filter and optionally cancel subsequent payloads whose path points to a location that has been nulled by error bubbling.

Status

• defined in spec
• implemented in graphql-js

[robrichard]

Interesting. In the current graphql-js implementation we will not initiate the deferred or streamed payloads if alwaysThrows throws synchronously, but the behavior you describe does happen if alwaysThrows is asynchronously rejected.

We discussed defer/stream acting as a "null-boundary" in #23, but we only discussed the scenario of the error inside of the defer/stream payload, not errors outside the payload.

@benjie Do you have any suggestions on how to address this?

[robrichard]

stream/defer selections shouldn’t start executing until all the ancestor selection sets have completed successfully

My concern with this approach is that this could significantly slow down the defer/stream selections. I know that almost by definition these selections are not as important for performance as the rest of the data, but I think there could be other negative effects if adding defer/stream to a query causes the total execution time to extend drastically.

The story around defer & stream has always been about not extending the execution time, but reducing latency to the non-deferred data, and there's some assumptions we've made around that. One example, is combining defer/stream with subscriptions. Adding defer to a subscription could cause a backlog of the payloads from subsequent subscription events if the execution time is extended.

[benjie]

For query and mutation operations, whether we specify in the algorithm that it starts during or after the main (non-deferred) selection execution is not observable to the client (except, arguably, as latency), so I wouldn't worry too much about that - the implementation can make its own decision on when to start it so long as the observable result is the same (i.e. alwaysThrows throwing in the main selection will prevent the stream from being output).

Conformance requirements expressed as algorithms can be fulfilled by an implementation of this specification in any way as long as the perceived result is equivalent. Algorithms described in this document are written to be easy to understand. Implementers are encouraged to include equivalent but optimized implementations.
-- https://spec.graphql.org/draft/#sel-GABDLFABABjK1oE

You make a good point about subscriptions though - do we have to process the main selection set and all stream/defer payloads before being able to process the main selection set against the next event? I hadn't really thought about it and kind of assumed they'd all be mixed into the same streaming response, but that would allow for unbounded growth and is very much a DOS vector so I can certainly see why we shouldn't do that. I think my argument in the paragraph above covers this too, in the algorithm we start them afterwards, but an optimised implementation may choose to start them sooner so long as the results aren't output if the destination no longer exists (i.e. there's no difference in the observed payloads).

[glasser]

do we have to process the main selection set and all stream/defer payloads before being able to process the main selection set against the next event

My understanding of the current spec and graphql-js implementation is that each subscription event gets fully processed including all payloads before any messages from the next event get sent (though it can start executing the next event earlier). This was surprising when I saw it but probably is better than alternatives?

[yaacovCR]

I don’t think the current implementation starts executing a subsequent event until after all payloads from the first event r fetched, although the spec would allow it.

[yaacovCR]

Interesting. In the current graphql-js implementation we will not initiate the deferred or streamed payloads if alwaysThrows throws synchronously...

I think that is true for defer, but not for stream.

See test in graphql/graphql-js#3749 that fails if we pull out the filtering that you added within the synchronous pathway.

[robrichard]

Could we consider adding something to spec acknowledging this scenario is a possibility?

Servers may send additional responses with a path that references a field that may be null or removed due to error bubbling in a previous response. Clients should discard these responses.

Effectively we are allowing defer/stream to act as a null boundary in both directions now (we discussed the other direction in #23).

It seems like it's an edge case and I'm concerned working around it on the server would either be too burdensome or would drastically affect the performance of the server execution.

[robrichard]

@benjie I have a rough implementation that I think implements what you described.

graphql/graphql-js#3720

I'm not 100% sure it covers all cases and the algorithm could likely be improved, but I wanted to get something down to aid the discussion.

[yaacovCR]

This whole thing is a nice catch and a very interesting discussion -- and a lightning fast response!

In terms of the filtering as implemented in the PR, it looks like in this initial version, work is done in the successful case to ensure that all subsequent payloads paths are not nulled somewhere in the initial result.

@robrichard -- am I right? Do you envision that this filter/check might be minimized in future iterations to only error cases? Something like saving some state on error that is later checked?

[robrichard]

@yaacovCR We could probably run the filter only when the payload contains errors. That's still not optimal though because graphql-js doesn't keep track of which errors caused null bubbling or how far the nulls have bubbled upwards. I think that would be a much larger change.

[benjie]

Argh; I just wrote this graphql/graphql-js#3720 (comment) before reading that. I didn't know graphql-js doesn't have somewhere that can catch the paths of bubbled errors 😞 (I'm not very familiar with the graphql-js codebase.)

[robrichard]

The graphql-js PR has been updated significantly, copying my comment from there:

Now, as discussed in the WG today, I've added the logic to filter subsequent payloads at the point where the bubbled null is ultimately set. This no longer requires inspecting the payload response and should not add any overhead in the non-error case.

[robrichard]

I updated the title/description and will mark this as resolved now. This is now reflected in the spec edit PR and has been merged to graphql-js

[benjie]

Excellent work! 🚀