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.

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

Editorial changes for Event Streams #1099

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

Editorial changes for Event Streams #1099

wants to merge 2 commits into from

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Jun 6, 2024

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)

@leebyron leebyron requested review from benjie, michaelstaib, Keweiqu and a team June 6, 2024 19:01
Copy link

netlify bot commented Jun 6, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 75f10e0
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/66688b2792d3d600080d13cc
😎 Deploy Preview https://deploy-preview-1099--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@leebyron leebyron force-pushed the streams-editorial branch 2 times, most recently from bd84927 to 6c52b20 Compare June 6, 2024 19:03
@leebyron leebyron added ✏️ Editorial PR is non-normative or does not influence implementation labels Jun 6, 2024
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

I think this is a massively clarifying PR!

I got hung up for a bit on the three verbs ("complete normally", "complete with error" and "cancel") and in particular trying to figure out the practical difference between "complete normally" and "cancel", but I think they ultimately make sense. My main concern is ensuring we've handled all the situations:

stream event handler
sourceStream completes normally complete responseStream normally
sourceStream completes with error complete responseStream with error
sourceStream is cancelled unhandled, since can only occur from responseStream being cancelled ❔
sourceStream encounters error forces complete with error -> handled above
responseStream completes normally unhandled, since can only occur from sourceStream completing
responseStream completes with error unhandled, occurs from sourceStream completing with error
responseStream is cancelled cancel sourceStream
responseStream encounters error forces complete with error -> unhandled above

Is there a situation in which responseStream might encounter an error (other than from sourceStream encountering an error)? If so, this is currently unhandled, I think?

Are there any other ways for sourceStream to be cancelled? I assume user code cancelling it would be seen by us as "completes normally"? (If so, this might warrant a non-normative note?)

the subscription.
receive payloads for a subscription. This in turn also cancels the Source
Stream, which is a good opportunity to clean up any other resources used by the
subscription.
Copy link
Member

Choose a reason for hiding this comment

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

This is a concrete change from "may" cancel the source stream to "does" cancel the source stream.

We create the source stream in the CreateSourceEventStream algorithm, so it is definitely our responsibility to also ensure it is released (completed). This new wording covers this, the old wording was essentially incorrect. I support this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, the previous wording was too loose. Ultimately this is not observable behavior from an API consumer's point of view, so also not spec enforceable, but I think this language is much more clear. If a resulting stream consumer cancels, then it's certainly the case that the source stream must be made aware of that.

Extra unnecessary detail in case if ever comes up: if an implementer doesn't want to cancel the source stream, they should be using an intermediate like tee

@benjie
Copy link
Member

benjie commented Jun 7, 2024

Oh I merged in the latest main by the way.

@leebyron
Copy link
Collaborator Author

leebyron commented Jun 11, 2024

@benjie thanks for the detailed thoughts, responding to your comment further above:

in particular trying to figure out the practical difference between "complete normally" and "cancel"

The practical difference is the origin and "direction" of the event. A completion is a stream emitter communicating that no further emit events will occur (either because the stream is completed normally, or because of an error). However a cancel is a stream consumer communicating that they are no longer interested in receiving events. The behavior described here is that after a stream emitter receives a cancel that it should pass that event along "backwards" relative to the flow of events. There is no "forwards" version, so there is no unhandled case of the source stream cancelling

Is there a situation in which responseStream might encounter an error (other than from sourceStream encountering an error)? If so, this is currently unhandled, I think?

You are correct, it's missing that ExecuteSubscriptionEvent could result in an error. I may need to investigate what we actually do in this scenario today since it may be that some errors are fatal and should "complete with error" the result stream and cancel the source stream, but others should be a normal emit event but describe the error — I'll investigate...

Are there any other ways for sourceStream to be cancelled? I assume user code cancelling it would be seen by us as "completes normally"? (If so, this might warrant a non-normative note?)

That's right, and good suggestion.


revising your table, these are the four observable events from the perspective of creating responseStream:

stream event handler
sourceStream emits value call ExecuteSubscriptionEvent. response stream emits value if no exception, otherwise completes with error and cancels sourceStream
sourceStream completes normally complete responseStream normally
sourceStream completes with error complete responseStream with error
responseStream is cancelled cancel sourceStream, complete responseStream

and these are removed from your table, they are not observable events from this perspective:

stream event handler explanation
sourceStream is cancelled not an observable event. responseStream as an observer sees completion.
sourceStream encounters error only observable as completion with error.
responseStream completes normally our responsibility to emit as creator of responseStream
responseStream completes with error our responsibility to emit as creator of responseStream
responseStream encounters error responseStream itself is not capable of creating errors. Relevant scenarios for encountering errors are handled above: passing through complete with error from sourceStream or handling errors during emit value event

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)
Comment on lines +307 to +309
- If internal {error} was raised:
- Cancel {sourceStream}.
- Complete {responseStream} with {error}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the note below, but I checked with graphql-js and this aligns. Request errors occur before field execution begins, which for subscriptions would be before source stream is created. Then each event through the subscription could produce field errors, but those are handled normally as { data, errors } and shouldn't complete the subscription. The only remaining scenario is some internal exceptional event.

I thought about continuing to leave this omitted, since it makes sense why it was omitted before, but I do agree that this is more clear about what to do in this non-normative scenario, and will make it more clear in the future if we ever decide to formally add "fatal error" as an error type

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% clear on what kind of internal error is being envisioned here. It seems like it's clearly not a request or field error, as explicitly stated above.

Is this analogous to the case of the graphql-js call to asyncIterator.next() resulting in an exception? I would think that's covered below when discussing "completing in error" but maybe that's not the case?

Of course, anything could error => maybe it just means "something else went wrong?" And we are making how to handle that more explicit in the case of a subscription than we would with regard to our other operation, because it is helpful to clients to understand how this case is handled in the context of the response stream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yaacovCR that last bit you said is exactly right. The vast amount of our spec assumes algorithms are pretty typical functions which you call with arguments and return a value, and the most significant complication is the introduction of async functions (which the spec really leaves mostly unstated), so the novelty here is the fact that this algorithm is not returning the result but returning a stream which will contain results. That's important because for a typical function we can just say if there is some exceptional error in any given algorithm that it should work its way up until something explicitly handles it or it fatals the whole operation - that's how error handling works (in some way or another) in just about every language and environment.

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.

Is this analogous to the case of the graphql-js call to asyncIterator.next() resulting in an exception?

Yes - but specifically inside where it is being mapped. graphql-js handles field errors during execution of a single subscription payload, resulting in a {data: null, errors} normal payload, but other exceptions could trigger this codepath. That's really what's being described here

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

This is much more descriptive and precise; excellent work! 🙌

- When {sourceStream} completes normally:
- Complete {responseStream} normally.
- When {sourceStream} completes with {error}:
- Complete {responseStream} with {error}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this wording imply that when the {responseStream} completes with error, it must do so in the same manner as when the {sourceStream} does? We have a request at graphql-js for emitting well formated GraphQLErrors instead of throwing. I would read the specification permissively, as in the manner in which the {responseStream} completes with said error is up to the implementation, allowing that change.

Once could argue instead/additionally that it belongs within the transport protocol specification.

Tagging @leebyron @enisdenjo @aseemk from the linked issue.

Copy link
Member

@enisdenjo enisdenjo Oct 14, 2024

Choose a reason for hiding this comment

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

From the SSE perspective, and in the GraphQL over SSE RFC, errors are emitted over the stream as an event.

This is because browser native EventSource will not report back to the user non-200 responses in detail - it'll just say "error" - leaving the user in the dark. Furthermore, if the error occurs after a while during the stream, the connection is already established and the only way to report an issue is in form of an event.

Same thing applies for WebSockets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants