When sourceStream errors, yield a { errors: [...] } response
#1127
Conversation
This makes a few changes to the Subscriptions section where we're talking about event streams in an attempt to make it more clear about what's going on. - Revised variable names from "fieldStream" to "sourceStream" to make it easier to trace variables through algorithms. - Rewrote the "Event Streams" definition to be more clear about "emit" keyword and have clear paragraphs on completion and cancellation. - Rewrote the `MapSourceToResponseEvent` algorithm to be a correctly formatted algorithm with a return statement at the end. Introduced a new "When" keyword to describe event subscriptions. Added explicit sections on passing back cancellation (discussed in WG) as well as completion with error (not discussed, but I realized was also left ambiguous)
|
Closing in favour of an inline edit |
|
Re-opened since #1099 is editorial whereas this is a behavioural change. |
|
tried a quick implementation at graphql/graphql-js#4302 |
benjie
changed the title
sourceStream errors, raise an error payload insteadsourceStream errors, yield a { errors: [...] } response
| - Complete {responseStream} with {error}. | ||
| - Let {errors} be a list containing {error}. | ||
| - Let {response} be an unordered map containing {errors}. | ||
| - Emit {response} on {responseStream}. | ||
| - Complete {responseStream} normally. |
There was a problem hiding this comment.
Follow up discussion about whether we need an error event in subscriptions. @benjie @yaacovCR @enisdenjo
From #1099 (comment) :
what about our own code and it's unexpected exceptions?
Our own code is perfect, no exception, never 👌🙂
Jokes aside, I guess there's always the possibility of exceptions such as out of memory, etc...
But there's no language in the spec for this for regular queries (or is there?). Why would there be for subscriptions?
From graphql/graphql-js#4001 (comment)
for example the Nginx proxy reboots, terminating all connections cleanly with an error; or the wifi drops, terminating all connections uncleanly - and we should differentiate these as best we can.
Isn't this out of the realm of the GraphQL spec? If TCP/PHY transport fails well, that's too bad but it's another layer. GraphQL is only concerned about its own applicative layer?
The fact that graphql-sse has been working without an error event seems to be indication that we don't really need this?
There was a problem hiding this comment.
Isn't this out of the realm of the GraphQL spec?
Yes, 100%. We cannot mandate that intermediaries generate GraphQL-shaped responses. In fact, we would expect them NOT to do that. That's why making sure that our own errors are formatted like GraphQL errors is so important: so that you know it hasn't originated from an intermediary.
This is why we introduced application/graphql-response+json in the HTTP spec, so that we don't need to HTTP 200 on error. Without that, intermediaries could produce errors in JSON format without them being GraphQL errors, and clients may get confused/crash. The GraphQL response type meant that clients could be sure they originated from GraphQL.
There was a problem hiding this comment.
But there's no language in the spec for this for regular queries (or is there?). Why would there be for subscriptions?
Redirecting to @leebyron comment from #1099 (comment)
where he addresses this:
However it's a lot less obvious what should happen for a stream which encounters an exceptional error after that particular creation algorithm has completed - what is the right behavior? This tries to be explicit to avoid that ambiguity - we should make sure the resulting stream also ends with an error.
Why is it less obvious? Well, one could have made an argument that in the spirit of GraphQL partial data, GraphQL should send a well formatted errors object for that event, and continue processing the response stream. But no! We do the more conservative thing and just end the stream with an error…
There was a problem hiding this comment.
one could have made an argument that in the spirit of GraphQL partial data, GraphQL should send a well formatted errors object for that event, and continue processing the response stream.
We do the more conservative thing and just end the stream with an error…
I'm going to advocate for the 3rd solution: send a well formatted errors object for that event, and complete the response stream normally (which is what this PR does).
This avoids the cognitive load associated with "completes with error" and keeps transport simple (no need for error event, just like graphql-sse).
Users that want to convey additional error information (error code, quota, retry delay, etc...) can do so in error.extension but this is outside the GraphQL spec.
There was a problem hiding this comment.
I like ^ this. Because there's not always a metadata place (conveying the severity of an error) across all streaming options.
There was a problem hiding this comment.
My assumption was that an internal error is a spec bug or a bug/unexpected condition within the implementation of the spec (based on @leebyron's comment).
It sounds like @martinbonnin you are suggesting changing:
- Let {response} be the result of running
{ExecuteSubscriptionEvent(subscription, schema, variableValues,
sourceValue)}.
- If internal {error} was raised:
- Cancel {sourceStream}.
- Complete {responseStream} with {error}.
to
- Let {response} be the result of running
{ExecuteSubscriptionEvent(subscription, schema, variableValues,
sourceValue)}.
- If internal {error} was raised:
- Cancel {sourceStream}.
- Let {errors} be a list containing {error}.
- Let {response} be an unordered map containing {errors}.
- Emit {response} on {responseStream}.
- Complete {responseStream} normally.
But what if an internal {error} is raised within these spec lines themselves:
- Cancel {sourceStream}.
- Let {errors} be a list containing {error}.
- Let {response} be an unordered map containing {errors}.
- Emit {response} on {responseStream}.
- Complete {responseStream} normally.
In that case, presumably our response stream will complete with that {error}, at least in languages that allow for that construct. For example, in Javascript, the call to next() on the iterator for the response stream will throw . I believe what @leebyron is suggesting is just that this also applies to an internal error raised by {ExecuteSubscriptionEvent()}. I don't see a particular advantage in producing a well-formed {errors} GraphQL response for an internal error raised by {ExecuteSubscriptionEvent()} when we don't do the same for an internal error within other algorithms (e.g. among several, ExecuteQuery()}, although I would say this is not particularly harmful, as @leebyron points out in that comment, internal errors raised within deeper logic could end up being wrapped as field errors.
Basically, because internal errors could happen anywhere, we may have to allow for the fact that our response stream might complete with an {error}. But that's not the expected case, and I am not actually sure for example if that maps to the transport level. So I am not actually sure whether this language needs to be in the spec, or maybe an accompanying note.
There was a problem hiding this comment.
Thanks @yaacovCR for the detailed comment 🙏
Wrapping the internal/unexpected errors from ExecuteSubscriptions() in a valid GraphQL response is indeed what I had in mind. I agree the asymmetry with ExecuteQuery() isn't great and feels off and might need revisiting.
All in all, I think I'd be happy not mentioning "internal errors" at all. If something unexpected comes up then behaviour is unspecified.
A query might:
- send HTTP 500
- send an {errors} GraphQL response
- terminate the TCP connection
- leave the TCP connection hanging (not great)
- ...
A subscription might:
- send a specific
errorevent - send a WebSocket
closeframe (single connection) - send an {errors} GraphQL response
- terminate the TCP connection
- leave the TCP connection hanging (not great)
- ...
It's not great but it's the cold reality of software. If things do not go according to plan well... we can't plan for them.
As of today, we have only one instance of "internal error" in the spec. It's very specific about errors in user resolvers and I wouldn't personally give the term more importance (see also Glossary PR).
Mentioning "internal errors" for the GraphQL code could erode the trust in the algorithms (why would there be an internal error there in the first place?) but also, what if there is an internal error while processing the internal error? And how should that error be represented?
Those are interesting questions but maybe better left out for the GraphQL over HTTP/WebSocket specs?
There was a problem hiding this comment.
If you leave that out, then does it mean that if the iterator ever throws an error that implementation is not spec compliant?
There was a problem hiding this comment.
@benjie depends what iterators we're talking about?
My current view is that errors in user code must always be wrapped. Those are errors in resolvers for queries/mutations/subscriptions as well as errors raised by the source stream for subscriptions.
Other errors are unhandled. Unhandled errors are spec compliant but the spec doesn't say anything about how they are represented. Similar to HTTP where unhandled errors can be HTTP 500 or something else.
GraphQL has errors/raising/handling. JS has exception/throwing/catching. Other languages may have other terminology.
There was a problem hiding this comment.
send a WebSocket close frame (single connection)
Note that the close message in the frame is limited in size.
spec/Section 6 -- Execution.md
Outdated
| - Let {response} be the result of running | ||
| {ExecuteSubscriptionEvent(subscription, schema, variableValues, | ||
| sourceValue)}. | ||
| - If internal {error} was raised: |
There was a problem hiding this comment.
Should implementations be allowed to recover (in cases where this is possible), returning the error within the response stream and continuing?
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Timestamped link to this being discussed at the GraphQL WG last week: https://youtu.be/6bW3Xcks5I4?t=3268 I'm going to leave this open to gather more feedback. I think we should bump it to RFC1 though; so I'm going to add it to the Jan WG. Forgot to ask at the last one whilst rushing through the schedule! |
See https://github.com/graphql/graphql-spec/pull/1099/files#r1799509253
Currently if
sourceStreamgenerates an error, thenresponseStreamrepeats the error. This is the behavior implemented in graphql-js and is problematic.GraphQL captures execution errors and wraps them in an
{ errors: [...] }payload for query and mutation operations; it should do the same for stream errors in a subscription operation.This PR makes this change. It is technically a breaking change, I think, so will require careful thought.