Fix SystemStackError when cache_fragment is used inside a mutation - #144
Open
LRFalk01 wants to merge 1 commit into
Open
Fix SystemStackError when cache_fragment is used inside a mutation#144LRFalk01 wants to merge 1 commit into
LRFalk01 wants to merge 1 commit into
Conversation
Tracer#execute_query's ensure block calls after_query (and so Cacher.call) for every query, mutations included, right as run_eager returns. Whether that's safe depends on whether context.fragments has anything in it yet. cache_fragment always wraps its result in a LazyCacheResolver, a registered lazy type. graphql-ruby only resolves lazy values inline, before run_eager returns, when the field is eager, and Interpreter::Runtime#run_eager sets is_eager = ast_node.operation_type == "mutation". So for mutations, LazyCacheResolver#resolve (which pushes the fragment into context.fragments) has already run by the time execute_query's ensure fires. For queries, that same resolution is deferred to a later phase outside execute_query's boundary, so context.fragments is still empty when the early hook fires, and Cacher.call's own guard (return unless query.context.fragments?) quietly no-ops. When context.fragments isn't empty, Cacher.call reaches Fragment#final_value, which calls context.query.result. Query#result checks @executed and, if it's still false, runs Execution::Interpreter.run_all(@Schema, [self], ...) to compute it. @executed only becomes true later, in run_all's own result_values= assignment loop, which runs after the whole multiplex's dataloader has drained -- well after execute_query has already returned for that query. So at the point execute_query's ensure calls into Cacher.call for a mutation, @executed is still false, and .result kicks off a brand new run_all for the same query. That re-enters execute_multiplex and execute_query, firing the same ensure again, with @executed still false, forever. execute_multiplex's own loop already calls after_query for every query in the multiplex, and only after super returns, by which point result_values= has been set for all of them. The execute_query-level call is redundant with that and is the only path that can fire while @executed is still false. Removing it fixes the crash without otherwise changing behavior: the full existing suite still passes, and standardrb is clean. Adds two specs covering a mutation with a synchronous cache_fragment and one with cache_fragment on a graphql-batch value. Both raise SystemStackError on master and pass with this fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
cache_fragmentinside a mutation's response type raisesSystemStackError: stack level too deep. It doesn't need a lazy value, a particular cache store, or nested fragments to trigger, justcache_fragmentanywhere under a mutation root field. The same call under a query root is fine.This should close #135. It may be related to #142.
Why
Tracer#execute_query'sensureblock callsafter_query(and soCacher.call) for every query as soon asrun_eagerreturns, mutations included. That's only safe ifcontext.fragmentsis still empty at that point, and for mutations it isn't.cache_fragmentwraps its result in aLazyCacheResolver, which graphql-ruby treats as a lazy value. Lazy values only get resolved inline, beforerun_eagerreturns, when the field is eager, andrun_eagermarks mutation root fields eager (is_eager = ast_node.operation_type == "mutation"). So by the timeexecute_query'sensurefires for a mutation, the fragment has already been pushed intocontext.fragments. For a query, that same resolution happens later, outsideexecute_query's boundary, so the early hook finds nothing andCacher.call's own guard skips it.Once
context.fragmentshas something in it,Cacher.callreachesFragment#final_value, which callscontext.query.result.Query#resultchecks@executed, and if it's false, it kicks off a freshExecution::Interpreter.run_allfor that same query to compute it.@executeddoesn't flip to true until later, inrun_all's own result-assignment loop, well afterexecute_queryalready returned. So for a mutation,.resultis called before that flag is set, triggers a whole new run, and that run hits the sameexecute_queryhook again, with the flag still false. It never stops.The fix
execute_multiplexalready loops over every query and callsafter_queryon each one, and it does that aftersuperreturns, by which point@executedis true for all of them. Theexecute_query-level call is redundant with that, and it's the only one that can fire early. Removing it gets rid of the crash without changing any other behavior.Testing
Added two specs: a mutation with a plain synchronous
cache_fragment, and one withcache_fragmenton a graphql-batch value (closer to how we actually hit this in production). Both raiseSystemStackErroron master and pass with the fix. Full suite is 121/121,standardrbis clean.