Sidekiq: Remove Sidekiq::Testing.inline!

[nateberkopec]

Semi-troll incoming! I realize this is unlikely, but wanted to get your thoughts.

I propose that Sidekiq::Testing.inline! is almost entirely misused, and just allows people to write slow, lazy tests.

The nightmare scenario is this: user puts Sidekiq jobs into ActiveRecord callbacks, then uses something like factory_girl and creates hundreds of records and thus sidekiq jobs on every test. There is no feedback that tons of Sidekiq jobs are being executed besides the tests being slow. Now the user's unit tests take minutes to complete.

Now, before you say "oh that's just a dumb user, provide sharp tools, etc etc" just look at how people actually use inline!.

Is there any use-case for inline which cannot be replaced by SomeWorker.drain?

Proposals:

• If inline is used, provide some logger feedback that jobs are being executed, and maybe how long they're taking. Sort of like the feedback provided when using delay on instance methods.
• Remove inline testing entirely.
• Remove the inline! method and only allow the block form of inline, which should eliminate some of it's misuse (though people will still use around blocks I bet)

[mperham]

Compelling pitch. I have never used inline myself, I think because it always had a vague smell to it. Historically there's been two reasons why people have used it outside of testing:

1. Don't have to run separate Sidekiq and Redis processes in development.
2. By running inside puma, they got Sidekiq::Worker code reloading (not necessary with Rails 5 anymore)

[nateberkopec]

Don't have to run separate Sidekiq and Redis processes in development.

Ah, ok, that explains all the usage in config/initializers. I think that's a valid use case. I wouldn't do it, but I get it. Any solution provided here could be mis-used for testing, though, so I don't know how would satisfy one and not the other. Maybe just a RACK_ENV-based warning.

[mperham]

https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/testing.rb#L329

[nateberkopec]

Yeah I guess what I'm proposing is the opposite of that warning - using inline! in tests. You could also move inline! outside of the Testing class.

[mperham]

What about a Sidekiq::Development.inline! API + removing the support for block-less Sidekiq::Testing.inline!?

Side note: Sidekiq needs an explicit API to get the current environment.

[nateberkopec]

That would definitely be better. Add some wiki stuff about why inline is bad/dangerous in tests, and you're probably set.

[j3pic]

This is an absolutely horrible idea! I'm trying to get Testing.inline! to work. The instructions in the wiki simply don't work. All the Sidekiq jobs go to Redis instead.

I'm working on a codebase that has exactly the nightmare scenario described here (ActiveRecord callbacks spawning Sidekiq jobs), and I can't tell if the callbacks even work in development unless I run the whole Sidekiq server locally (and if an error happens in the Sidekiq server, it'll be very difficult to debug)! There isn't even a way to do something like Sidekiq::Queue.pop.perform because the things in the queue are just these incomprehensible serialized monstrosities (that aren't serialized with Marshal.dump so I have no idea how to deserialize them) that don't look like they even have the parameters that are supposed to be being passed to the jobs.

[thbar]

@mperham Just chiming in to state I personally like what you proposed. FWIW, my use cases with inline are:

• Mostly, using the explicit block form in in-browser integration tests: this kind of documents the fact that something is expected to be going on in the background, without the need to be super specific with regard to which types of jobs are running
• Running inline sidekiq jobs inside rake db:seed (using Sidekiq::Development.inline! in the future instead of the current version), which is convenient for data-sync heavy apps I work on (this is more a hack, but a very convenient one!)

Apart from that, yes, the use of ActiveRecord callbacks are one of the biggest pains faced by the "maintainer jumping on an unknown codebase", sadly :smile:.

[ryansch]

We use the block form of inline! quite a bit in our feature specs.

Example:

Sidekiq::Testing.inline! do
  WorkerThatSpawnsOtherJobs.drain
end

[mperham]

Is someone interested in providing a PR which logs inline job runs with timing? I don't want to remove APIs at this point but more information (even at DEBUG level) would be useful.

[ayanko]

user puts Sidekiq jobs into ActiveRecord callbacks

That is development/architecture fault of this user.

Why we should remove Sidekiq functionality due to this "issue" ?

Please leave Sidekiq::Testing.inline! - we want to have full integrations tests in capybara spec/features

[mikegee]

we want to have full integrations tests in capybara spec/features

Its arguably not "full" integration if you don't push the work to Redis and run them from a Sidekiq worker process.

[mzagaja]

Using inline! in development is super handy. I haven't tried using it in my actual tests but after reading the wiki it does seem like the first approach of using drain is a bit more comprehensive.

[seanfcarroll]

I vote for leaving it in: it is useful in both dev and tests.

[nateberkopec]

I don't want to remove APIs at this point but more information (even at DEBUG level) would be useful.

Something similar to the warning that was added in Sidekiq 5 re: job size arguments would work.

# if we are using Sidekiq::Testing.inline! 
::Sidekiq.logger.debug { "#{@target}.#{name} took #{time_elapsed}" }

I still advocate for the original solution of Sidekiq::Development.inline! API + removing the support for block-less Sidekiq::Testing.inline!.

[mperham]

We have 5 years of codebases using Sidekiq and I don't want to break APIs without a clean, documented migration path and a very good reason. Are there specific changes anyone recommends and would be willing to prototype? Without a specific proposal, I'm going to close this issue; we all agree -- "this could be better" -- but what is better enough to justify the migration pain?

[mperham]

When in doubt, don't poke a sleeping bear. Too much inertia here so we'll leave it alone.