This seems to be a flaw in the design. I see no reason why the subscription support should not return errors generated while executing a subscription data event (which is much like a document executer). The specification clearly says that errors should be returned.
http://spec.graphql.org/October2021/#sec-Response-Stream
Suggestion: return execution errors just like a document executer would
2. Execution errors, extensions, and metrics are not cleaned up after a data eventEach data event that generated errors would get added to the ExecutionContext.Errors instance, and never cleared. A similar issue would happen if metrics were enabled. Essentially this is a memory leak, and potentially a bad one if metrics were enabled along with the middleware.
Suggestion: reinitialize errors, output extensions and metrics from the shared context after they are returned within the execution result (if necessary).
3. Exceptions generated by a document listener are not handled by the unhandled exception handlerSuggestion: process exceptions by the unhandled exception handler
4. Errors generated by the observable source are not handled by the unhandled exception handlerSuggestion: process exceptions by the unhandled exception handler
5. Default error descriptions are inaccurate, and contain the entire original querySuggestion: fix, and remove entire original query from error message
6. If the initial request contains multiple subscription requests, the context is sharedThis could lead to problems like errors from one subscription data event being returned within another subscription data event. I'm not actually sure that the GraphQL spec allows for multiple subscriptions at once, but regardless, it seems like a design flaw in the code.
Suggestion: clone the ExecutionContext for each individual subscription field requested; executions within that subscription occur sequentially so the ExecutionContext can be shared over multiple data events
7. The cancellation token provided to field resolvers for data events reflects the http cancellation token and not the subscription cancellation token (set when the subscription is disposed of)The graphql-ws protocol allows for starting and stopping subscriptions without breaking the websockets connection. So the cancellation token for the http context may be longer than that of the subscription. In practice this won't matter much as the websocket handler should request disconnection from the event stream source and if an extra event gets through, the websocket handler should ignore it.
Suggestion: Assuming the ExecutionContext has been cloned as stated above, the cancellation token within the execution context can be set within each data event to a CreateLinkedCancellationToken of the http cancellation token and subscription cancellation token.
Note: Unlike the document executer, upon an OperationCancelledException, the code should not throw; back to the observer source, but rather should produce a simple execution result with a 'operation cancelled' message. This may happen under normal circumstances if a disconnection or stop request happens exactly at the same time that the event source sends data. Such an event should not throw back to the event stream source.
While this is good in practice, and works as intended, it seems to be flawed in design. It is not the job of the subscription execution engine to know what the lifetime of the RequestServices property is or to manage it.
Consider that it is only because we changed the implementation within the server project to create a scope for the request that we wanted to clear it at all. The change in the server project was correct, but fixing a side-effect within the GraphQL.NET project seems incorrect.
It is certainly possible to execute the subscription (for its whole lifetime) with the RequestServices property of the http context. It is not disposed of until the http connection is broken. It may not be practical for some DI-based frameworks like EF, but it's still a valid configuration.
Note that the RequestServices property is currently not cleared for field resolvers executed when data from the event stream is resolved.
Suggestion 1: Do not clear the RequestServices property. If it is used after the initial request, an ObjectDisposedException will occur from the DI provider (since the server project will have disposed of the scope by then). Consequence is that there is an unneeded reference to the service provider hanging around for the lifetime of the subscription, even if it is disposed properly. (I would assume that the DI provider would clear its managed resources properly upon disposal and we need not worry about this.)
Suggestion 2: We could have ExecutionContext hold a reference to the ExecutionOptions instance that configured it, and so the server project can set ExecutionOptions.RequestServices to null at the appropriate time, which would effectively set all lingering ExecutionContext and ReadonlyResolveFieldContext instances to have a null RequestServices property also. However, this probably has other consequences, like preventing a document listener from setting RequestServices, and preventing fixing issue 9 below.
9. There is no way to configure a service scope for the lifetime of a data event only, provided within RequestServices, and 10. There is no way to use a serial execution strategy for data events within a subscriptionMy GraphQL frameworks heavily rely on scoped services and serial execution, as the scoped services (e.g. Entity Framework) do not support multi-threaded use. When I start using subscriptions shortly, I would very much prefer to have the data events' node trees executed serially with RequestServices set to a new service scope for the duration of the data event only. In fact I would have to say that much of my code would probably not work unless that change could be made.
Suggestion 1: Provide a single class SubscriptionExecutionStrategy with an argument in the constructor to specify serial or parallel execution.
Suggestion 2: Provide two classes, ParallelSubscriptionExecutionStrategy and SerialSubscriptionExecutionStrategy.
In either case, a user can override one of the methods within the strategy to create a service scope and change the context's RequestServices property during a data event. Perhaps within MicrosoftDI we provide a ScopedSubscriptionExecutionStrategy implementations.
11. Output extensions are not returned for data eventsSuggestion: Return extensions, if any, along with the response
12. Metrics may be captured but not returned on data eventsSuggestion 1: Disable metrics for all data events (set context to Metrics.None or similar).
Suggestion 2: Perform metrics on data events same as a document executer would. It is up to the server project to "EnrichWithApolloTracing" on each event if desired. (Tricky because the context does not know if ExecutionOptions.UseMetrics was set to true or false)
Suggestion 3: Provide a way to configure between the two suggestions above, perhaps a constructor argument
13. SubscriptionExecutionResult is somewhat unnecessary and requires a cast to useImplementations must cast the execution result from the DocumentExecuter to SubscriptionExecutionResult to be able to retrieve the Streams property.
Suggestion: Move the Streams property from SubscriptionExecutionResult to ExecutionResult - #3024
Suggestion 2: Leave as-is.
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4