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

Removed use of defer #104

Merged
merged 2 commits into from
Jun 5, 2015
Merged

Removed use of defer #104

merged 2 commits into from
Jun 5, 2015

Conversation

gshutler
Copy link
Contributor

@gshutler gshutler commented Jun 4, 2015

Following sidekiq's lead this is no longer necessary. It also means that Shoryuken::Shutdown will be raised on the worker thread which, in turn means it is possible to create middleware that respond to it.


What lead me here was looking for a way to react to Shoryuken::Shutdown within middleware. This is so that I can set a low visibility timeout which means the messages will become visible much quicker for reprocessing.

The use of defer means that the worker is executing in a second thread and so Shoryuken::Shutdown was being sent to its parent instead.

As an aside, using less threads will likely be better for performance too.

The only downside I can imagine for this is that any custom middleware with a blanket rescue will start catching these previously uncatchable errors. However, it's pretty easy to ignore them if this is a problem with:

rescue Shoryuken::Shutdown
  raise
rescue => e
  # whatever

Following [sidekiq's lead][1] this is no longer necessary. It also means
that `Shoryuken::Shutdown` will be raised on the worker thread which, in
turn means it is possible to create middleware that respond to it.

 [1]: sidekiq/sidekiq#919
@gshutler
Copy link
Contributor Author

gshutler commented Jun 4, 2015

In case it's useful, here's a middleware that makes use of Shoryuken::Shutdown:

class ShutdownRequeueMiddleware
  def call(worker, queue, sqs_msg, body)
    yield
  rescue Shoryuken::Shutdown
    set_visible(sqs_msg)
    raise
  end

  private

  def set_visible(sqs_msg)
    sqs_msg.change_visibility(visibility_timeout: 0)
  rescue
    # fail silently
  end
end

@phstc
Copy link
Collaborator

phstc commented Jun 4, 2015

@gshutler nice 🍻

continuous-integration/travis-ci/pr — The Travis CI build failed

I need to fix Travis. This error "unable to sign request without credentials set" is because a missing configuration in Travis. But this one "expected: 1 time with arguments: (15)" is a real one , I can dig into that.

Use of defer was necessary when Celluloid message dispatch was done within fibers. This requires twice the number of threads. Remove it now that thread dispatch is the default.

@sschepens @esebastian maybe this change can help with the memory leak problem #88.

@phstc
Copy link
Collaborator

phstc commented Jun 4, 2015

Found the problem, every is no longer working without defer: https://github.com/phstc/shoryuken/blob/master/lib/shoryuken/processor.rb#L42

I've also tried timers directly with no luck. The only way I could make it work was using a second actor. Not sure if I'm happy with that yet.

Anyway, I'm still debugging it 😄

@phstc
Copy link
Collaborator

phstc commented Jun 4, 2015

Closing in favor of #105

@phstc phstc closed this Jun 4, 2015
@gshutler
Copy link
Contributor Author

gshutler commented Jun 5, 2015

Ah, sorry. I didn't notice that some of the specs weren't running with a plain rspec call.

Removing defer meant that the timer never got a chance to run, therefore
it needs its own actor.
@phstc phstc reopened this Jun 5, 2015
@phstc phstc merged commit 82e888a into ruby-shoryuken:master Jun 5, 2015
@phstc
Copy link
Collaborator

phstc commented Jun 5, 2015

@gshutler awesome! great work. Added to master.

@esebastian
Copy link

@phstc Great, I'll take it for a spin and keep you informed!

@gshutler
Copy link
Contributor Author

gshutler commented Jun 5, 2015

Excellent, thanks for your help on it @phstc (and Shoryuken as a whole)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants