search

arthurnn / memcached

A Ruby interface to the libmemcached C client
Academic Free License v3.0
432 stars 127 forks source link

Some setters and getters inappropriately accept a non-String key #105

Closed jason-o-matic closed 11 years ago

jason-o-matic commented 11 years ago

I've packaged up a bunch of logic around producing cache keys into my own key object, but right now I need to throw .to_s on these objects in some cases.

It seems like all of the methods should take any object that responds to to_str.

Here's my simplified example of what I'm trying to do.

1.9.3p194 :001 > class SomeKey
1.9.3p194 :002?>   def to_s
1.9.3p194 :003?>     "foo"
1.9.3p194 :004?>     end
1.9.3p194 :005?>   alias :to_str :to_s
1.9.3p194 :006?>   end
 => nil 
1.9.3p194 :007 > k = SomeKey.new
 => foo 
1.9.3p194 :008 > $memcache.get(k)
 => nil 
1.9.3p194 :009 > $memcache.set(k, "bar")
 => true 
1.9.3p194 :010 > $memcache.get(k)
 => "bar" 
1.9.3p194 :011 > $memcache.get_multi([k])
TypeError: wrong argument type SomeKey (expected String)
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/memcached.rb:495:in `memcached_mget'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/memcached.rb:495:in `get'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/rails.rb:83:in `get_multi'
    from (irb):11
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands/console.rb:47:in `start'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands/console.rb:8:in `start'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:12:in `require'
    from script/rails:12:in `<main>'
1.9.3p194 :012 > $memcache.get([k])
TypeError: wrong argument type SomeKey (expected String)
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/memcached.rb:495:in `memcached_mget'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/memcached.rb:495:in `get'
    from (eval):4:in `get_with_instrumental_trace'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/memcached-1.4.3/lib/memcached/rails.rb:50:in `get'
    from (irb):12
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands/console.rb:47:in `start'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands/console.rb:8:in `start'
    from /Users/jason/projects/newtoy/gwf_service_copy/vendor/bundle/ruby/1.9.1/gems/railties-3.2.8/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:12:in `require'
    from script/rails:12:in `<main>'
evan commented 11 years ago

Adding type coercion would be a performance regression, sorry. The real issue might be, why do the gets appear to work at all?

In my personal opinion this kind of programming style leads to bugs. How does the gem know your to_s is unique or even deterministic?

jason-o-matic commented 11 years ago

I wouldn't think the gem would care if my to_str was unique or deterministic. If it wasn't I would have bugs, and bugs would be expected until I fixed my code. Essentially, whatever object I pass in would have to act enough like a string to work.

I was guessing that since the gets worked that there was some additional checking happening on get_multi that could be removed, but maybe that's not the case?

Where would the performance regression come from? Couldn't you treat the incoming object like a string without actually checking that it is a string?

evan commented 11 years ago

Perhaps, but assuming its a string, rescuing the exception, and recovering would very expensive. It would lead to unpredictable performance in practice. A slight change from a string to an object could hose your efficiency.

I need to investigate why non-strings are accepted at all, really.

jason-o-matic commented 11 years ago

Do you really need to rescue the exception? I would expect it to blow up if I wasn't passing in something that passed as a string.

evan commented 11 years ago

Ok, you are right, and I am wrong. Ruby convention demands that I honor to_str and a fast macro is provided for the purpose in the C API. It won't regress performance.