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.
-
|
My thoughts to your remarks: So, yes, I know that this implementation is not complete, and that it didn't fully follow the specification. :) But still I wanted to give it a go. And yes, it's a basic implementation that is not ready for use. I still believe it's promising in that it could be made viable with some special constraints. Tackling your different remarks: Naming the thingSo we could call it JSON streaming, but in fact the implementation could use some other form of streaming. But we could call it something like "output streaming", "lowlevel streaming", "syntactic streaming" (as opposed to higher level or logical level streaming in the queries). Because to me this is still streaming the response on some OutputStream channel. But for the purpose of the discussion let's stick to JSON Streaming. Error HandlingHere I believe this could be handled by collecting errors in memory along the way (as likely done today) and generating the errors (if any at the end). If a fatal error that breaks further processing then the JSON stream of data would have to be properly balanced in closing what has been opened but not yet closed. But are there such cases ? If you have any tests which you could point me to that would be cool. Then maybe I could simulate such situations. Null handlingIn a first step I would, as you suggested, "accept" that in case of streaming the bubbling of nulls be "ignored" and left to the client choosing JSON streaming with this difference in behavior as a counter-part. However, I think it would be possible to analyse the path info to check if there are any non-nullable steps in the selection sets and delay the streaming until the point where the null vs non-null question if determined. There would be a memory impact here as you would need to "remember". This would E880 be tricky, but I don't believe it would be impossible. AsynchronicityThis code assumed that "completion" is done in order as it seems to be when adding tracing to the logs even if async data fetchers are being used. But agreed this could be just a coincidence. I was assuming fetching happened async but completion not. Is there no order guarantee in completion or somewhere else ? (even "by accident" ? I mean is there absolutely no order tracking somewhere that could lead to the fact that completion does end up being in order not by design but as a consequence of some other ordering constraint ?) If there is no such sync point then indeed it would need to be added to the implementation to ensure proper ordering of the results (only stream when my parents have opened and my previous siblings have completed). Likely parents could pilot the streaming of their outputs. |
Beta Was this translation helpful? Give feedback.
-
|
@bbakerman Is the null bubbling only in case of null on a non-nullable field ? I can't seam to find an example in the test cases where that's not the case. |
Beta Was this translation helpful? Give feedback.
-
|
Yes it only happens when the runtime returns null to a field that is declared as non null. It adds an error it bubbles backwards to parent to it to null as the contract has been broken. It's actually a PITA to implement since it involves back tracking on work you have already done. eg nominally a grpahql engine is a eager depth based tree descent - from parent to child fields as they get values - but this null bubbling means that previous work has to be discarded. As you say one could imagine this could be turned "off" in a world where the data was streamed out as it arrived |
Beta Was this translation helpful? Give feedback.
-
|
Ok, then to me it's completely acceptable to lift this constraint as in fact, this means that the data fetchers do not respect the contact of the schema. To me this is a implementation error and should not happen at runtime. If you say in your schema that this field is non null but then you send null, then you're contradicting yourself. Do you know how this ended up in the spec ? Schema and implementation should be consistent. |
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.
Uh oh!
There was an error while loading. Please reload this page.
-
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