Implement #marshal_load and #marshal_dump for Twitter::Identity

[trliner]

In version 5.2.0 of the gem, marshaling tweets, users, and other objects that inherit from Twitter::Identity raises an exception, which makes certain things difficult, like storing them in Rails.cache.

Here's a simple example to reproduce the exception:

require 'twitter'

client = Twitter::REST::Client.new do |config|
  config.consumer_key        = "YOUR_CONSUMER_KEY"
  config.consumer_secret     = "YOUR_CONSUMER_SECRET"
  config.access_token        = "YOUR_ACCESS_TOKEN"
  config.access_token_secret = "YOUR_ACCESS_SECRET"
end

tweet = client.user_timeline('gem').first
Marshal.dump(tweet)
# => TypeError: no _dump_data is defined for class Proc

This commit defines marshal_dump and marshal_load for the Twitter::Identity class. Unfortunately, the test I wrote doesn't fail without those methods because rspec-mocks redefines Marshal.load. So any feedback on how to write a better test (or on the pull request in general) would be appreciated.

[trliner]

It looks like the build failed in Travis CI because the YARD-Coverage percentage increased. Am I reading that wrong?

[sferik]

It looks like the build failed in Travis CI because the YARD-Coverage percentage increased. Am I reading that wrong?

@trliner No, you’re reading that correctly. If you increase the documentation coverage, you need to increase the documentation threshold in the Rakefile. This might sound crazy but it ensures that documentation regressions do not occur.

Thank you for increasing this project’s documentation coverage!

[sferik]

Is there any particular reason you decided to implement these methods on Twitter::Identity instead of Twitter::Base?

If you look at the entity-relationship diagram for this library, you can see that many classes inherit directly from Twitter::Base (Twitter::Configuration, Twitter::Entity, Twitter::Geo, Twitter::Language, etc.).

I am still undecided about this patch, but it seems like if you’re able to marshal any Twitter::Identity object, you should be able to marshal any Twitter::Base object, too.

[sferik]

Oh, now I see why you applied this patch to Twitter::Identity instead of Twitter::Base. It appears that objects that mixin an Equalizer instance cannot be marshaled and raise a cryptic error when you try:

TypeError: no _dump_data is defined for class Proc

This seems like an issue that should be addressed in the equalizer gem.

@dkubb @mbj @solnic: Thoughts?

[sferik]

Actually, I take that back. I’m not sure this is an equalizer bug. I need to investigate further. I assumed the issue was equalizer because the implementation of Twitter::Identity is so thin: https://github.com/sferik/twitter/blob/c555b8af0ce58df8bd0eafd294cd3fb1417c9eb6/lib/twitter/identity.rb

It looked to me like equalizer was the only difference between Twitter::Base objects (which can be marshaled) Twitter::Identity objects (which cannot) but I’m having trouble producing a reduced example.

[dkubb]

@sferik maybe a gist of a stacktrace would help. Offhand I can't recall anywhere that uses a Proc object in equalizer, but a stacktrace might allow us to narrow things down more easily.

[sferik]

Unfortunately, there is no stack trace to speak of:

irb(main):001:0> require 'twitter'
=> true
irb(main):002:0> b = Twitter::Base.new(:id => 1)
=> #<Twitter::Base:0x007fc8ab1c9730 @attrs={:id=>1}>
irb(main):003:0> Marshal.dump(b)
=> "\x04\bo:\x12Twitter::Base\x06:\v@attrs{\x06:\aidi\x06"
irb(main):004:0> i = Twitter::Identity.new(:id => 1)
=> #<Twitter::Identity id=1>
irb(main):005:0> Marshal.dump(i)
TypeError: no _dump_data is defined for class Proc
    from (irb):5:in `dump'
    from (irb):5
    from /Users/e/.rbenv/versions/1.9.3-p484/bin/irb:12:in `<main>'

[dkubb]

I wonder if it's disappearing because some of the work is happening inside C code. Maybe try the same thing with rbx?

[sferik]

@dkubb Here’s the stack trace on Rubinius:

irb(main):001:0> require 'twitter'
=> true
irb(main):002:0> b = Twitter::Base.new(:id => 1)
=> #<Twitter::Base:0x16634 @attrs={:id=>1}>
irb(main):003:0> Marshal.dump(b)
=> "\x04\bo:\x12Twitter::Base\x06:\v@attrs{\x06:\aidi\x06"
irb(main):004:0> i = Twitter::Identity.new(:id => 1)
=> #<Twitter::Identity id=1>
irb(main):005:0> Marshal.dump(i)
TypeError: marshaling is undefined for class Proc
    from kernel/common/marshal.rb:351:in `__marshal__'
    from kernel/common/marshal.rb:911:in `serialize'
    from kernel/common/marshal.rb:967:in `serialize_instance_variables_suffix'
    from kernel/bootstrap/array.rb:66:in `each'
    from kernel/common/marshal.rb:959:in `serialize_instance_variables_suffix'
    from kernel/common/marshal.rb:8:in `__marshal__'
    from kernel/common/marshal.rb:911:in `serialize'
    from kernel/common/marshal.rb:967:in `serialize_instance_variables_suffix'
    from kernel/bootstrap/array.rb:66:in `each'
    from kernel/common/marshal.rb:959:in `serialize_instance_variables_suffix'
    from kernel/common/marshal.rb:8:in `__marshal__'
    from kernel/common/marshal.rb:911:in `serialize'
    from kernel/common/marshal.rb:1157:in `dump'
    from (irb):5
    from kernel/common/block_environment.rb:53:in `call_on_instance'
    from kernel/common/eval.rb:176:in `eval'
    from kernel/common/kernel.rb:440:in `loop'
    from kernel/bootstrap/proc.rb:20:in `call'
    from kernel/common/throw_catch.rb:30:in `catch'
    from kernel/common/throw_catch.rb:8:in `register'
    from kernel/common/throw_catch.rb:29:in `catch'
    from kernel/bootstrap/proc.rb:20:in `call'
    from kernel/common/throw_catch.rb:30:in `catch'
    from kernel/common/throw_catch.rb:8:in `register'
    from kernel/common/throw_catch.rb:29:in `catch'
    from /Users/e/.rbenv/versions/rbx-2.2.1/gems/gems/rubysl-irb-2.0.4/bin/irb:12:in `__script__'
    from kernel/common/kernel.rb:427:in `load'
    from /Users/e/.rbenv/versions/rbx-2.2.1/bin/irb:23:in `__script__'
    from kernel/delta/code_loader.rb:66:in `load_script'
    from kernel/delta/code_loader.rb:198:in `load_script'
    from kernel/loader.rb:649:in `script'
    from kernel/loader.rb:831:in `main'

[sferik]

Okay, this is officially one of the most infuriating bugs I have ever seen. As I was debugging it, I condensed the failure case onto a single line and…it magically worked. When I then split it into multiple lines, it fails again. What the actual fuck?

irb(main):001:0> require 'twitter'
=> true
irb(main):002:0> i = Twitter::Identity.new(:id => 1); Marshal.dump(i)
=> "\x04\bo:\x16Twitter::Identity\x06:\v@attrs{\x06:\aidi\x06"
irb(main):003:0> i = Twitter::Identity.new(:id => 1)
=> #<Twitter::Identity id=1>
irb(main):004:0> i.is_a?(Proc)
=> false
irb(main):005:0> Marshal.dump(i)
TypeError: no _dump_data is defined for class Proc
    from (irb):5:in `dump'
    from (irb):5
    from /Users/e/.rbenv/versions/1.9.3-p484/bin/irb:12:in `<main>'
irb(main):006:0> i = Twitter::Identity.new(:id => 1); Marshal.dump(i)
=> "\x04\bo:\x16Twitter::Identity\x06:\v@attrs{\x06:\aidi\x06"
irb(main):007:0> # WTF?!?

[sferik]

Okay, this does appear to have something to do with equalizer. If I comment out line 6 of lib/twitter/identity.rb, I am unable to reproduce the issue. That said, I’m still unable to reduce this error so it occurs outside the context of the twitter gem. I will keep working on that…

[mbj]

@sferik Your repro above belongs to current master?

[paracycle]

@sferik I made one more observation, this works:

[1] pry(main)> i = Twitter::Identity.new(:id => 1);
[2] pry(main)> Marshal.dump(i)
=> "\x04\bo:\x16Twitter::Identity\x06:\v@attrs{\x06:\aidi\x06"

So I guess someone is adding a Proc when inspect is called and that is the difference between your two cases.

It seems inspect is a method dynamically added by equalizer:

[10] pry(main)> show-source i.inspect

From: /Users/ufuk/.rvm/gems/ruby-1.9.3-p194@twitter-gem/gems/equalizer-0.0.7/lib/equalizer.rb @ line 68:
Owner: #<Equalizer:0x007fc75892c300>
Visibility: public
Number of lines: 5

define_method(:inspect) do ||
  klass = self.class
  name  = klass.name || klass.inspect
  "#<#{name}#{keys.map { |key| " #{key}=#{send(key).inspect}" }.join}>"
end

Still this does not explain where the Proc comes from, though.

[sferik]

I’ve finally produced a reduced example. This seems to be caused by an interaction between equalizer and memoizable version 0.2.0. If I run this same code with memoizable version 0.3.0, it miraculously works (in both cases):

require 'equalizer'
require 'memoizable'

class WithoutEqualizer
  include Memoizable
  attr_reader :id
  memoize(:id)
  def initialize(id)
    @id = id
  end
end

class WithEqualizer
  include Equalizer.new(:id)
  include Memoizable
  attr_reader :id
  memoize(:id)
  def initialize(id)
    @id = id
  end
end

without = WithoutEqualizer.new(1)
with    = WithEqualizer.new(1)
p Marshal.dump(without) # works as expected
p Marshal.dump(with)    # raises TypeError with memoizable 0.2.0; works as expected with memoizable 0.3.0

[paracycle]

@sferik I think I got this:

[1] pry(main)> i = Twitter::Identity.new(:id => 1);
[2] pry(main)> ls i
Memoizable::InstanceMethods#methods: freeze  memoize
Twitter::Base#methods: []  attrs  to_h  to_hash  to_hsh
Equalizer::Methods#methods: ==  eql?
#<Equalizer:0x007fa90580e548>#methods: hash  inspect
Twitter::Identity#methods: id  id?
instance variables: @attrs
[3] pry(main)> i
=> #<Twitter::Identity id=1>
[4] pry(main)> ls i
Memoizable::InstanceMethods#methods: freeze  memoize
Twitter::Base#methods: []  attrs  to_h  to_hash  to_hsh
Equalizer::Methods#methods: ==  eql?
#<Equalizer:0x007fa90580e548>#methods: hash  inspect
Twitter::Identity#methods: id  id?
instance variables: @_memoized_method_cache  @attrs
[5] pry(main)> i.instance_variable_get :@_memoized_method_cache
=> #<Memoizable::Memory:0x007fa90414df98
 @freezer=
  #<Proc:0x007fa905c70960@/Users/ufuk/.rvm/gems/ruby-1.9.3-p194@twitter-gem/gems/memoizable-0.2.0/lib/memoizable.rb:13 (lambda)>,
 @memory=
  #<ThreadSafe::Cache:0x007fa90414df70 @backend={:id=>1}, @default_proc=nil>>

As you can see, until inspect is called, we don't have a @_memoized_method_cache instance variable. That instance variable actually has a @freezer which is a Proc.

So I guess this is a problem with memoizable and not necessarily with equalizer, since memoizable is that one adding a Proc. It might be doing that under different circumstances as well.

[paracycle]

@sferik Oops, a few minutes late. 😃

[sferik]

So now I know how to fix the bug but maybe someone can explain to me why the error would consistently occur when this code was on two separate lines:

i = Twitter::Identity.new(:id => 1)
Marshal.dump(i)

But not on one line, with the statements separated by a semicolon:

i = Twitter::Identity.new(:id => 1); Marshal.dump(i)

I thought it might be a timing issue, but I can’t reproduce it like this either:

i = Twitter::Identity.new(:id => 1); sleep 1; Marshal.dump(i)

I also can’t reproduce the error by running the code from a file, only via irb (or pry).

[paracycle]

@sferik I did try to explain here and here

Actually it even works this way (at least in pry):

i = Twitter::Identity.new(:id => 1); 
Marshal.dump(i)

and the difference is in the extra call to inspect which accesses id, thus triggering memoizable.

[paracycle]

@sferik If you run the Ruby file:

require 'twitter'

i = Twitter::Identity.new(:id => 1)
puts i.id
Marshal.dump(i)

you also get an exception since id is accessed.

[sferik]

@paracycle Thank you. It all makes sense now. Wow.

@dkubb What do you think about adding a regression test to memoizable to ensure that memoized objects are serializable with Marshal? I hope no one has to struggle with this issue ever again. I wouldn’t wish this bug on my worst enemy.

[paracycle]

@sferik Specifically, it is dkubb/memoizable@c77b8ab by @dkubb that removed the @freezer instance variable of Memoizable::Memory thus fixing the Marshal.dump problem.

Note: Updated link to commit

[sferik]

@paracycle I believe it was actually this commit: dkubb/memoizable@c77b8ab.

[paracycle]

@sferik Yes, I had a copy/paste problem there for a second. I had updated the comment to point to the same commit. 😃

Interesting side effect of a commit that is made for completely different reasons.

[sferik]

I don’t even know where to start writing a regression test. This is probably the most salient, real-world example I’ve encountered of why dynamic languages are dangerous (specifically, dynamic method definition). Think I’m going to go write some Haskell now.

[trliner]

Hmm, that fixed the issue for Twitter::Base, but marshaling Twitter::Identity still throws a (different) TypeError.

[1] pry(main)> require 'twitter'
=> true
[2] pry(main)> Twitter::Version.to_s
=> "5.3.0"
[3] pry(main)> Memoizable::VERSION
=> "0.3.1"
[4] pry(main)> b = Twitter::Base.new(:id => 1)
=> #<Twitter::Base:0x007fb23f0c25d0 @attrs={:id=>1}>
[5] pry(main)> Marshal.dump(b)
=> "\x04\bo:\x12Twitter::Base\x06:\v@attrs{\x06:\aidi\x06"
[6] pry(main)> i = Twitter::Identity.new(:id => 1)
=> #<Twitter::Identity id=1>
[7] pry(main)> Marshal.dump(i)
TypeError: no _dump_data is defined for class Mutex
from (pry):7:in `dump'

Interestingly, marshaling without saving to a variable does works:

[8] pry(main)> Marshal.dump Twitter::Identity.new(:id => 1)
=> "\x04\bo:\x16Twitter::Identity\x06:\v@attrs{\x06:\aidi\x06"

[sferik]

I’ve opened an issue against the memoizable project to address this: dkubb/memoizable#10.

[sferik]

Note, the error is slightly different than before:

TypeError: no _dump_data is defined for class Mutex

It was:

TypeError: no _dump_data is defined for class Proc

The issue was fixed in memoizable 0.3.0 but regressed in 0.3.1 as a result of this commit, which added a Monitor (which is implemented using a Mutex).

This patch was added to fix a ThreadError in memoizable that was ultimately caused by this issue in thread_safe, where check-then-act methods, such as ThreadSafe::Cache#compute_if_absent, cannot be used recessively. Even if that issue is resolved, its resolution will likely involve adding a Mutex, so I’m not sure how this can be fixed as long as memoizable uses ThreadSafe::Cache. This data structure could easily be replaced by a Hash but then the code is not thread safe on Ruby implementations without a GIL (e.g. JRuby).