Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librato-sidekiq drops other middlewares #7

Open
ragalie opened this issue May 27, 2014 · 3 comments · Fixed by meetcleo/librato-sidekiq#1 · May be fixed by #8
Open

librato-sidekiq drops other middlewares #7

ragalie opened this issue May 27, 2014 · 3 comments · Fixed by meetcleo/librato-sidekiq#1 · May be fixed by #8

Comments

@ragalie
Copy link

ragalie commented May 27, 2014

On the first job invoked by a sidekiq process, the librato-sidekiq middleware will drop (cause sidekiq not to execute) the middleware immediately after it in the chain. Here's why it happens:

When a job is ready to be invoked, Sidekiq.server_middleware.invoke is called with the relevant worker instance and arguments. #invoke maps over the middleware chain and instantiates new middleware objects of the correct classes (dups the chain, essentially).

Each time the librato-sidekiq middleware is initialized it "reconfigures" itself, which really means that it removes and re-adds itself to the middleware chain entries array. Array#each expects that the position of objects in the array will not change during iteration. Since the librato-sidekiq middleware removes an element of the entries array while the chain is mapping over that array, the entry immediately following ends up "skipped" (and the librato-sidekiq middleware runs twice).

Is there a reason why #reconfigure has to be run every time the middleware is instantiated? Can we move the method to the class and just run it once after .configure is run?

The offending code is here: https://github.com/StatusPage/librato-sidekiq/blob/master/lib/librato-sidekiq/middleware.rb#L46-54

For reference, here is the sidekiq class involved: https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/middleware/chain.rb

And here is the logic for Array#each: http://www.ruby-doc.org/core-2.1.2/Array.html#method-i-each

@scootklein
Copy link
Contributor

Can you check out the https://github.com/StatusPage/librato-sidekiq/tree/middleware_drops branch? Need to fix tests, but I think this will solve it for you.

ragalie pushed a commit to ragalie/librato-sidekiq that referenced this issue Jun 4, 2014
There's no reason to add/remove the middleware from the middleware
chain everytime a sidekiq job runs. Only do it when the options change.

Fixes StatusPage#7
@ragalie ragalie linked a pull request Jun 4, 2014 that will close this issue
@ragalie
Copy link
Author

ragalie commented Jun 4, 2014

@scootklein I think your change has unintended side effects. If the middleware has already been added and you attempt to change the options via configure, those options will not take effect.

I opened a PR (#8) for an alternative change that only runs .reconfigure when .configure is called instead of every time a sidekiq job runs. Can you take a look and see if it makes sense to you?

Thanks!

@thibaudgg
Copy link

👍 That one is really annoying, lost many hours of debugging another middleware.

thibaudgg pushed a commit to thibaudgg/librato-sidekiq that referenced this issue Sep 18, 2014
There's no reason to add/remove the middleware from the middleware
chain everytime a sidekiq job runs. Only do it when the options change.

Fixes StatusPage#7

Conflicts:
	lib/librato-sidekiq/middleware.rb
pfigel pushed a commit to pfigel/librato-sidekiq that referenced this issue Sep 15, 2017
There's no reason to add/remove the middleware from the middleware
chain everytime a sidekiq job runs. Only do it when the options change.

Fixes StatusPage#7

Conflicts:
	lib/librato-sidekiq/middleware.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants