Inconsistent behaviour in InlineExecutor vs DefaultExecutor

[archaeologistdev]

After several hours of debugging, I've discovered a subtle yet important difference between the two executor implementations.

Shoryuken::Worker::DefaultExecutor.perform_in relies on the worker class' implementation of perform_async:

def perform_in(worker_class, interval, body, options = {})
  # ...
  worker_class.perform_async(body, options.merge(delay_seconds: delay))
end

In contrast, Shoryuken::Worker::InlineExecutor.perform_in always calls its own implementation of perform_async:

def perform_in(worker_class, _interval, body, options = {})
  perform_async(worker_class, body, options)
end

Now, assume you have a custom Shoryuken.worker_executor (e.g. because you want to use to_json instead of JSON.dump)

class ToJsonExecutor
  class << self
    def perform_async(worker_class, body, options = {})
      Shoryuken::Worker::DefaultExecutor.perform_async(worker_class, body.to_json, options)
    end

    def perform_in(worker_class, interval, body, options = {})
      Shoryuken::Worker::DefaultExecutor.perform_in(worker_class, interval, body.to_json, options)
    end
  end
end

This is wrong and will call body.to_json twice. The same code will work fine if you replace DefaultExecutor with InlineExecutor, which is confusing.

Which fix would make more sense?

1. change DefaultExecutor.perform_in to be self contained (calling its own implementation of perform_async instead of going via the worker class) or
2. change InlineExecutor.perform_in to delegate to the worker class

[phstc]

Hi @ocisly

Thanks for investigating that and the well detailed explanation.

For a PR, I would go with option 2.

BTW if the body parser wasn't just an example, I would recommend you to use body_parser instead.

[archaeologistdev]

Thanks! I've opened a PR for option 2.

I don't think body_parser is an option here, because I need to control the serialisation of the message body before it is sent. I guess client middleware could work?

[archaeologistdev]

Just looked into that. client middleware wouldn't work out of the box, because sanitize_message! is called before the middleware gets invoked.