Replies: 4 comments 4 replies
-
Ping @bbakerman @andimarek : If I understand correctly, the "specification" requires having everything in memory, but I still think that the memory savings would be interesting when the response payloads are quite big. Also, I used instrumentation here, but my guess is that we could provide an execution strategy instead no ? |
Beta Was this translation helpful? Give feedback.
-
(FYI: I use jackson generator in this case, but changing the StreamingState we could just as well use something else, e.g. SAX handlers so we could keep it serialization agnostic) |
Beta Was this translation helpful? Give feedback.
-
So I had a look at the code and this is a very interesting approach. It has indeed piqued my interest and the idea has some merit. Its is indeed JSON streaming or perhaps better naming might be JSON generation. Now things can get confusing here because there is a graphql I think it's important to differentiate the two, otherwise people will get confused. What you are doing in this code is "JSON streaming" the results as they arrive. As each field completion is started, you are making a JSON write of the field data (aka So the nice thing about this is that in theory you don't need to hold all the graphql result data in memory. As each field completes, it would be written to the JsonGenerator and would not need to be held in memory for a later result object. (And as you say right now you can't stop the ExecutionResult being built but one could imagine such a state). Its interesting to contrast this approach versus the https://graphql.org/blog/2020-12-08-defer-stream/
Some challenges I see with this approach are : Error and extensions handlingErrors are meant to be delivered in a Non null handling when nullThe graphql spec says that if a non nullable field ends up null at runtime, and error should be raised and its parent field should be made null https://spec.graphql.org/October2021/#sec-Handling-Field-Errors This bubbles up all the way to the top according to spec
This would be impossible to do in this case becase the JSON generator would have already had a value written to it in and hence its impossible to undo. I guess this would be the price of such JSON generation - this spec requirement of non null error bubble up would have to be forgone. Async field completionWith the code as is today, I suspect the start of a field needs to be match with the end of the field. That is fields complete in perfect order.
The code opens a JSON attribute on say the but in an async world I suspect the JSON generation is going to get mixed up badly. I suspect the idea of async field completion might be very hard to do given the current approach. |
Beta Was this translation helpful? Give feedback.
-
For the asynchronous part I'm trying to think of a way to provoke out if order completions in a unit test. Any ideas ? If I can find a case where this happens, it'll make it easier for me to implement reordering. |
Beta Was this translation helpful? Give feedback.
-
Hello all,
I wanted to share a little experiment I did to show the feasibility of streaming the GraphQL result during the execution of a GraphQL query.
Here is what I can up with: https://gitlab.com/bhabegger/graphql-streaming
In this PoC the result is both streamed through the instrumentation and returned as usual. So no memory gain, but this was just to show the feasibility. As the ExecutionResult-s are no longer needed, to save memory they could be freed up along the way.
Tell me what you think !
Ping @dfa1
Beta Was this translation helpful? Give feedback.
All reactions