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

Unused instance variable #19

Closed
mrduncan opened this issue Dec 11, 2009 · 5 comments
Closed

Unused instance variable #19

mrduncan opened this issue Dec 11, 2009 · 5 comments
Labels

Comments

@mrduncan
Copy link
Contributor

The @watched_queues instance variable in resque.rb is read but never written to.

The following commit removes it entirely:
mrduncan/resque@298125b9b1aa29d6214e64d44e7803cbacc17193

@defunkt
Copy link
Contributor

defunkt commented Dec 11, 2009

This variable is used. I've added a comment to explain: 0f296e3

Cheers!

@mrduncan
Copy link
Contributor Author

Unless I'm missing something, it looks like @watched_queues is only being read. I can't find anywhere that it's actually being set.

def watch_queue(queue)
  @watched_queues ||= {}
  return if @watched_queues[queue]  # <-- @watched_queues is always empty here
  redis.sadd(:queues, queue.to_s)
end

It seems like the following line should be added to actually cache the queue:

def watch_queue(queue)
  @watched_queues ||= {}
  return if @watched_queues[queue]
  redis.sadd(:queues, queue.to_s)
  @watched_queues[queue.to_s] = true  # <-- Cache it
end

Also, if the queue parameter is ever anything other than a string (I haven't checked to see if that actually ever happens), the following change should probably be made to avoid cache misses:

def watch_queue(queue)
  @watched_queues ||= {}
  return if @watched_queues[queue.to_s]  # <-- Get string for consistency
  redis.sadd(:queues, queue.to_s)
  @watched_queues[queue.to_s] = true
end

@defunkt
Copy link
Contributor

defunkt commented Dec 12, 2009

You're right, looks like this needs a bugfix and a testcase.

@mrduncan
Copy link
Contributor Author

I made an attempt at a fix (mrduncan/resque@6e4233ea6d0b8cba6068f9bb05253470fd0f6253).

The only issue that I see with it is if a queue is removed it could still be in the cache of another process - seems like the Redis set which holds queue names might not get an entry for that queue if a new job is pushed.

@defunkt
Copy link
Contributor

defunkt commented Dec 15, 2009

I think you're right about the other processes caching queues. Since this has never actually worked, it's probably a case of premature optimization. I removed it in 02a6ac1

Thanks!

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

No branches or pull requests

2 participants