Allow custom executors #68

chrisbutcher · 2017-11-06T22:45:50Z

Changed: GraphQL::Batch.use can take optional custom_executor
- GraphQL::Batch::Setup and GraphQL::Batch::SetupMultiplex updated to hold reference to the executor class (defaults to GraphQL::Batch::Executor) as instance variables.

@dylanahsmith @eapache @swalkinshaw 👋 ~~I'm looking to verify my approach here before continuing. See the linked Shopify/shopify PR for more context.~~ Updated according PR review comments so far.

dylanahsmith

I had considered passing in the executor into the Setup class initializers, but then noticed that we call start_batching here, not from an instance but from another class method: https://github.com/Shopify/graphql-batch/blob/master/lib/graphql/batch/setup.rb#L30

That method is deprecated, so we don't need to support a custom executor there

dylanahsmith · 2017-11-06T23:47:51Z

lib/graphql/batch.rb

        instrumentation = GraphQL::Batch::SetupMultiplex.new(schema)
        schema_defn.instrument(:multiplex, instrumentation)
        schema_defn.instrument(:field, instrumentation)
      else
+        GraphQL::Batch::Setup.executor_class = executor_class
        instrumentation = GraphQL::Batch::Setup.new(schema)


Store the state on the instance rather than the class. It is only supposed to affect the schema it is called on.

dylanahsmith · 2017-11-06T23:49:38Z

lib/graphql/batch.rb

@@ -12,20 +12,22 @@ class NestedError < StandardError; end
    def self.batch
      raise NestedError if GraphQL::Batch::Executor.current
      begin
-        GraphQL::Batch::Executor.current = GraphQL::Batch::Executor.new
+        GraphQL::Batch::Executor.current = GraphQL::Batch::Executor.new # TODO


Changing this isn't important for our purposes, so you could just remove the # TODO comment.

chrisbutcher · 2017-11-07T15:59:56Z

Thanks @dylanahsmith ! I've updated the PR in response to your suggestions. Any suggestions on how/if we should unit test this change? We don't have a SetupTest, ~~nor coverage on GraphQL::Batch.use.~~

The only testable changes appear to be in GraphQL::Batch.use's forwarding of executor_class, and the expectation in start_batching that the passed-in Executor class instantiated.

dylanahsmith · 2017-11-07T16:55:27Z

Any suggestions on how/if we should unit test this change? We don't have a SetupTest, nor coverage on GraphQL::Batch.use.

Integration style tests would be preferred for something like that, since it is mean to be used through graphql-ruby rather than directly. We test GraphQL::Batch.use through use GraphQL::Batch right now

chrisbutcher · 2017-11-07T19:53:09Z

test/graphql_test.rb

+    query_string = '{ product(id: "1") { id } }'
+    schema.execute(query_string)
+
+    refute custom_executor.loaders_executed.empty?


Possibly related to https://github.com/Shopify/shopify/pull/136940#issuecomment-342542248?
I noticed that this ended up containing two references to the same instance of RecordLoader.

chrisbutcher · 2017-11-07T19:53:52Z

Updated. I ended up adding a gem dependency (mocha) to make assertions on some trivial behaviour defined on a custom executor.

swalkinshaw

LGTM but also wondering about the two loaders being executed

chrisbutcher · 2017-11-07T20:16:17Z

@swalkinshaw I'm looking into this now. It seems to be because we call the instrumented method twice via finish_resolve https://github.com/Shopify/graphql-batch/blob/master/lib/graphql/batch/loader.rb#L101, because we call finish_resolve in both fulfill and reject: https://github.com/Shopify/graphql-batch/blob/master/lib/graphql/batch/loader.rb#L69-L79

It does this regardless of whether an existing collection id is queried in the query mentioned here.

dylanahsmith · 2017-11-07T19:58:13Z

graphql-batch.gemspec

@@ -27,4 +27,5 @@ Gem::Specification.new do |spec|
  spec.add_development_dependency "bundler", "~> 1.10"
  spec.add_development_dependency "rake", "~> 10.0"
  spec.add_development_dependency "minitest"
+  spec.add_development_dependency "mocha", "~> 1.3.0"


I would like to try to avoid including a stubbing library, since they can result in tests passing only because of the presence of the stub. Integration tests provide more confidence that the code actually works.

dylanahsmith · 2017-11-07T20:03:39Z

test/graphql_test.rb

+    end
+
+    custom_executor = MyCustomExecutor.new
+    MyCustomExecutor.stubs(:new).returns(custom_executor)


I would prefer if you at least use a class variable for the loaders_executed array so that you don't need to mess with the implementation using stubbing.

dylanahsmith · 2017-11-07T20:06:21Z

test/graphql_test.rb

+    end
+  end
+
+  def test_custom_executor_class


I think this should move to another test class, since this one gets re-used in GraphQL::ExecutionStrategyTest which tests the deprecated code that we don't support custom executors for.

dylanahsmith · 2017-11-07T20:21:17Z

test/graphql_test.rb

+    end
+
+    def around_promise_callbacks(loader)
+      @loaders_executed << loader


This is called each time a promise is fulfilled, not for each loader being executed. E.g. if 4 keys are being loaded by a loader, then this will be called for all 4 keys rather than the one load.

This is called each time a promise is fulfilled, not for each loader being executed.

Thanks for the clarification!

It looks like around_promise_callbacks is also invoked via Loader#reject. So that's twice per field, even if the field is fulfilled. reject is called even when the loader finds a result, via check_for_broken_promises. This explains the double logging in my current implementation 🤔

lib/graphql/batch/loader.rb:78:in `reject' lib/graphql/batch/loader.rb:131:in `block in check_for_broken_promises' lib/graphql/batch/loader.rb:130:in `each' lib/graphql/batch/loader.rb:130:in `check_for_broken_promises' lib/graphql/batch/loader.rb:48:in `resolve' lib/graphql/batch/executor.rb:34:in `resolve' lib/graphql/batch/loader.rb:56:in `wait' lib/promise.rb:129:in `wait' lib/promise.rb:129:in `wait' lib/promise.rb:129:in `wait' lib/promise.rb:80:in `sync' lib/graphql/field.rb:309:in `public_send'

I was able to around the double-logging by resetting @start_time to nil in around_promise_callbacks and returning early if it's not present on next call, like so:

module GraphModel class TimedBatchExecutor < GraphQL::Batch::Executor # etc. def resolve(loader) @start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) super end def around_promise_callbacks(loader) return super unless @start_time end_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) TIMINGS[loader.loader_key] ||= [] TIMINGS[loader.loader_key] << end_time - @start_time @start_time = nil super end end end

This prevents the second call to around_promise_callbacks (via Executor#resolve -> Loader#resolve, #check_for_broken_promises, #reject, #finish_resolve) from logging the time between the original @start_time and the time it runs again.

chrisbutcher · 2017-11-08T16:11:58Z

Another follow-up change after some discussion with @swalkinshaw.

I think we can get around modifying around_promise_callbacks's signature to take a loader by simply keeping a reference to the loader we already receive in Executor#resolve. Will push up commit to https://github.com/Shopify/shopify/pull/136940 shortly.

chrisbutcher · 2017-11-08T16:48:01Z

Updated and ready for another review

swalkinshaw

squash and

swalkinshaw · 2017-11-08T17:42:21Z

lib/graphql/batch/setup_multiplex.rb

@@ -1,11 +1,12 @@
 module GraphQL::Batch
  class SetupMultiplex
-    def initialize(schema)
+    def initialize(schema, executor_class: executor_class)


🚨 hold up.

lib/graphql/batch/setup_multiplex.rb:3: warning: circular argument reference - executor_class

- def initialize(schema, executor_class: executor_class) + def initialize(schema, executor_class:)

✅

Pass custom executor as instance variable Expose loader to around_promise_callbacks Add Mocha gem Test passing custom executor_class Move custom_executor test to separate file Reverting change to around_promise_callbacks signature Fix circular reference

dylanahsmith suggested changes Nov 6, 2017

View reviewed changes

chrisbutcher changed the title ~~WIP Allow custom executors~~ Allow custom executors Nov 7, 2017

dylanahsmith approved these changes Nov 7, 2017

View reviewed changes

chrisbutcher commented Nov 7, 2017

View reviewed changes

chrisbutcher requested review from swalkinshaw and eapache November 7, 2017 19:54

swalkinshaw approved these changes Nov 7, 2017

View reviewed changes

dylanahsmith suggested changes Nov 7, 2017

View reviewed changes

chrisbutcher force-pushed the allow-custom-executor branch from 433b142 to 4a2d094 Compare November 7, 2017 21:35

dylanahsmith approved these changes Nov 8, 2017

View reviewed changes

eapache approved these changes Nov 8, 2017

View reviewed changes

swalkinshaw approved these changes Nov 8, 2017

View reviewed changes

swalkinshaw reviewed Nov 8, 2017

View reviewed changes

Allow custom executors

acf575c

Pass custom executor as instance variable Expose loader to around_promise_callbacks Add Mocha gem Test passing custom executor_class Move custom_executor test to separate file Reverting change to around_promise_callbacks signature Fix circular reference

chrisbutcher force-pushed the allow-custom-executor branch from f95884d to acf575c Compare November 8, 2017 18:18

chrisbutcher merged commit 09b136a into master Nov 9, 2017

chrisbutcher deleted the allow-custom-executor branch November 9, 2017 15:41

chrisbutcher had a problem deploying to rubygems November 9, 2017 15:48 Failure

chrisbutcher had a problem deploying to rubygems November 9, 2017 15:50 Failure

chrisbutcher deployed to rubygems November 9, 2017 15:54 Active

swalkinshaw mentioned this pull request Nov 13, 2017

Concurrency #45

Open

xuorig mentioned this pull request Dec 13, 2017

Allow a custom executor class in GraphQL::Batch.batch #75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom executors #68

Allow custom executors #68

chrisbutcher commented Nov 6, 2017 •

edited

Loading

dylanahsmith left a comment

dylanahsmith Nov 6, 2017

dylanahsmith Nov 6, 2017

chrisbutcher commented Nov 7, 2017 •

edited

Loading

dylanahsmith commented Nov 7, 2017

chrisbutcher Nov 7, 2017

chrisbutcher commented Nov 7, 2017

swalkinshaw left a comment

chrisbutcher commented Nov 7, 2017

dylanahsmith Nov 7, 2017

dylanahsmith Nov 7, 2017

dylanahsmith Nov 7, 2017

dylanahsmith Nov 7, 2017

chrisbutcher Nov 7, 2017 •

edited

Loading

chrisbutcher Nov 8, 2017 •

edited

Loading

chrisbutcher commented Nov 8, 2017 •

edited

Loading

chrisbutcher commented Nov 8, 2017

swalkinshaw left a comment

swalkinshaw Nov 8, 2017

chrisbutcher Nov 8, 2017

chrisbutcher Nov 8, 2017

Allow custom executors #68

Allow custom executors #68

Conversation

chrisbutcher commented Nov 6, 2017 • edited Loading

dylanahsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbutcher commented Nov 7, 2017 • edited Loading

dylanahsmith commented Nov 7, 2017

Choose a reason for hiding this comment

chrisbutcher commented Nov 7, 2017

swalkinshaw left a comment

Choose a reason for hiding this comment

chrisbutcher commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbutcher Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

chrisbutcher Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

chrisbutcher commented Nov 8, 2017 • edited Loading

chrisbutcher commented Nov 8, 2017

swalkinshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbutcher commented Nov 6, 2017 •

edited

Loading

chrisbutcher commented Nov 7, 2017 •

edited

Loading

chrisbutcher Nov 7, 2017 •

edited

Loading

chrisbutcher Nov 8, 2017 •

edited

Loading

chrisbutcher commented Nov 8, 2017 •

edited

Loading