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.

Sign up for GitHub

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

When sourceStream errors, yield a { errors: [...] } response #1127

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

When sourceStream errors, yield a { errors: [...] } response #1127

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5bb1e07
Editorial changes for Event Streams
leebyron Jun 6, 2024
75f10e0
feedback and use definition syntax
leebyron Jun 11, 2024
2ef903c
Don't complete with error.
benjie Nov 21, 2024
5257c92
Don't use a new algorithm, no need to DRY
benjie Nov 21, 2024
f49aab7
Internal errors should be passed up the chain
benjie Nov 21, 2024
1951969
Merge branch 'main' into streams-editorial-suggestions
benjie Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion spec/Section 6 -- Execution.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,10 @@ MapSourceToResponseEvent(sourceStream, subscription, schema, variableValues):
- When {sourceStream} completes normally:
- Complete {responseStream} normally.
- When {sourceStream} completes with {error}:
- 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.
Comment on lines -314 to +317
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor

@martinbonnin martinbonnin Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@enisdenjo enisdenjo Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like ^ this. Because there's not always a metadata place (conveying the severity of an error) across all streaming options.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 error event
  • send a WebSocket close frame (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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you leave that out, then does it mean that if the iterator ever throws an error that implementation is not spec compliant?

Copy link
Contributor

@martinbonnin martinbonnin Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send a WebSocket close frame (single connection)

Note that the close message in the frame is limited in size.

- When {responseStream} is cancelled:
- Cancel {sourceStream}.
- Complete {responseStream} normally.
Expand Down
Loading