Project

General

Profile

Bug #12353

Regression with Marshal.dump on ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]

Added by jeffreyc (Jeff C) over 3 years ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]
[ruby-core:75377]

Description

Attempting to call Marshal.dump on an ActiveSupport::CoreExtensions::Numeric::Time (e.g., 1.day) triggers a NoMethodError: undefined method `marshal_dump' for 86400:Fixnum exception in both Ruby 2.3.0 and 2.3.1 on OS X (10.11.4) and in Travis CI with ActiveSupport 4.1.15. This works correctly in previous versions, including 2.2.5.

To reproduce:

% rbenv install 2.3.1
...
% rbenv global 2.3.1
% gem install activesupport -v 4.1.15
...
% ruby --version    
ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]
% irb
irb(main):001:0> require 'active_support/core_ext/numeric/time'
=> true
irb(main):002:0> Marshal.dump(1.day)
NoMethodError: undefined method `marshal_dump' for 86400:Fixnum
    from ~/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activesupport-4.1.15/lib/active_support/duration.rb:115:in `method_missing'
    from (irb):2:in `dump'
    from (irb):2
    from ~/.rbenv/versions/2.3.1/bin/irb:11:in `<main>'

In 2.2.5:

% rbenv install 2.2.5
...
% rbenv global 2.2.5
% gem install activesupport -v 4.1.15
...
% ruby --version
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-darwin15]
% irb                                
irb(main):001:0> require 'active_support/core_ext/numeric/time'
=> true
irb(main):002:0> Marshal.dump(1.day)
=> "\x04\bo:\x1CActiveSupport::Duration\a:\v@valuei\x03\x80Q\x01:\v@parts[\x06[\a:\tdaysi\x06"

Associated revisions

Revision b51174a6
Added by nobu (Nobuyoshi Nakada) over 3 years ago

No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@55500 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 55500
Added by nobu (Nobuyoshi Nakada) over 3 years ago

No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

Revision 55500
Added by nobu (Nobuyoshi Nakada) over 3 years ago

No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

Revision 55500
Added by nobu (Nobuyoshi Nakada) over 3 years ago

No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

Revision 55500
Added by nobu (Nobuyoshi Nakada) over 3 years ago

No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

Revision 8dc8bba4
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

merge revision(s) 54142,55500: [Backport #12353]

    assertions.rb: fix result of assert_nothing_raised

    * test/lib/test/unit/assertions.rb (assert_nothing_raised): do not

discard the result of the given block.
* vm_method.c (vm_respond_to): try method_missing if respond_to?
is undefined, as if it is the default definition.
[ruby-core:75377] [Bug #12353]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_3@55869 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 55869
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

merge revision(s) 54142,55500: [Backport #12353]

assertions.rb: fix result of assert_nothing_raised

* test/lib/test/unit/assertions.rb (assert_nothing_raised): do not

discard the result of the given block.
* vm_method.c (vm_respond_to): try method_missing if respond_to?
is undefined, as if it is the default definition.
[ruby-core:75377] [Bug #12353]

Revision 53369d7e
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • vm_method.c: revert r55869. it breaks Integer#days with
    ActiveSupport-4.1.x. [ruby-core:76949] [Bug #12353]

  • test/ruby/test_marshal.rb: ditto.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_3@56022 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 56022
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • vm_method.c: revert r55869. it breaks Integer#days with
    ActiveSupport-4.1.x. [ruby-core:76949] [Bug #12353]

  • test/ruby/test_marshal.rb: ditto.

History

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Status changed from Open to Feedback

I can't reproduce it.

$ gem2.3 install --user --no-doc activesupport
Fetching: i18n-0.7.0.gem (100%)
Successfully installed i18n-0.7.0
Fetching: activesupport-4.2.6.gem (100%)
Successfully installed activesupport-4.2.6
2 gems installed

$ ruby2.3 -v -ractive_support/core_ext/numeric/time -e 'p Marshal.dump(1.day)'
ruby 2.3.1p112 (2016-04-26 revision 54768) [universal.x86_64-darwin15]
"\x04\bo:\x1CActiveSupport::Duration\a:\v@valuei\x03\x80Q\x01:\v@parts[\x06[\a:\tdaysi\x06"

Updated by jeffreyc (Jeff C) over 3 years ago

Nobuyoshi Nakada wrote:

I can't reproduce it.

Successfully installed activesupport-4.2.6

The regression appears to be specific to ActiveSupport 4.1.15. If I try ActiveSupport 4.2.6, Marshal.dump behaves as expected.

Updated by bakineggs (Dan Barry) over 3 years ago

Using Ruby 2.3, Marshal.dump(1.day) works with ActiveSupport 4.2.6 but not 4.1.15 because ActiveSupport::Duration inherits from BasicObject in 4.1.15 and Object in 4.2.6.

The issue is that as of Ruby 2.3, Marshal.dump no longer works with objects that inherit from BasicObject and do not directly define a respond_to? method. This is because vm_respond_to() in vm_method.c returns true if it can not find a method table entry for the respond_to? method, so w_object() in marshal.c tries to call marshal_dump. In Ruby 2.2 and prior, rb_obj_respond_to() would just call respond_to? instead of checking for a method table entry, so method_missing could receive the respond_to?.

Here's a simple example:

$ irb
2.3.1 :001 > class Wrapper < BasicObject
2.3.1 :002?>   def initialize wrapped_object
2.3.1 :003?>     @wrapped_object = wrapped_object
2.3.1 :004?>   end
2.3.1 :005?>   def method_missing *args, &block
2.3.1 :006?>     @wrapped_object.public_send *args, &block
2.3.1 :007?>   end
2.3.1 :008?> end
 => :method_missing 
2.3.1 :009 > wrapped_string = Wrapper.new "foo"
 => "foo" 
2.3.1 :010 > wrapped_string.respond_to? :marshal_dump
 => false 
2.3.1 :011 > Marshal.dump wrapped_string
NoMethodError: undefined method `marshal_dump' for "foo":String
    from (irb):6:in `public_send'
    from (irb):6:in `method_missing'
    from (irb):11:in `dump'
    from (irb):11
    from /Users/dan/.rvm/rubies/ruby-2.3.1/bin/irb:11:in `<main>'
2.3.1 :012 > class Wrapper
2.3.1 :013?>   def respond_to? *args, &block
2.3.1 :014?>     super
2.3.1 :015?>   end
2.3.1 :016?> end
 => :respond_to? 
2.3.1 :017 > wrapped_string.respond_to? :marshal_dump
 => false 
2.3.1 :018 > Marshal.dump wrapped_string
 => "\x04\bo:\fWrapper\x06:\x14@wrapped_objectI\"\bfoo\x06:\x06ET"

In Ruby 2.2 and earlier, Marshal.dump(wrapped_string) would work without needing to define a respond_to? method on Wrapper.

It's bad practice to define a method_missing without a corresponding respond_to_missing?, but even with a respond_to_missing? method defined, we still need a respond_to? in order for Marshal.dump to work:

2.3.1 :001 > class Wrapper < BasicObject
2.3.1 :002?>   def initialize wrapped_object
2.3.1 :003?>     @wrapped_object = wrapped_object
2.3.1 :004?>   end
2.3.1 :005?>   def method_missing *args, &block
2.3.1 :006?>     @wrapped_object.public_send *args, &block
2.3.1 :007?>   end
2.3.1 :008?>   def respond_to_missing? method_name, include_private = false
2.3.1 :009?>     @wrapped_object.respond_to? method_name, include_private
2.3.1 :010?>   end
2.3.1 :011?> end
 => :respond_to_missing? 
2.3.1 :012 > wrapped_string = Wrapper.new "foo"
 => "foo" 
2.3.1 :013 > wrapped_string.respond_to? :marshal_dump
 => false 
2.3.1 :014 > Marshal.dump wrapped_string
NoMethodError: undefined method `marshal_dump' for "foo":String
    from (irb):6:in `public_send'
    from (irb):6:in `method_missing'
    from (irb):14:in `dump'
    from (irb):14
    from /Users/dan/.rvm/rubies/ruby-2.3.1/bin/irb:11:in `<main>'

One way to achieve the expected behavior would be to change vm_respond_to() so that it calls method_missing(:respond_to?, priv) when it can't find a method table entry for respond_to? instead of just returning true.

I'd be happy to make this change if someone on the core team can let me know if my suggestion is the right way to handle this or if there's a different way things should be changed.

#4

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Status changed from Feedback to Closed

Applied in changeset r55500.


No respond_to? as if the default definition

  • vm_method.c (vm_respond_to): try method_missing if respond_to? is undefined, as if it is the default definition. [ruby-core:75377] [Bug #12353]

Updated by usa (Usaku NAKAMURA) about 3 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN to 2.1: DONTNEED, 2.2: DONTNEED, 2.3: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • Backport changed from 2.1: DONTNEED, 2.2: DONTNEED, 2.3: REQUIRED to 2.1: DONTNEED, 2.2: DONTNEED, 2.3: DONE

ruby_2_3 r55869 merged revision(s) 54142,55500.

Updated by knu (Akinori MUSHA) about 3 years ago

  • Status changed from Closed to Open

I found this backport broke ActiveSupport::Duration of ActiveSupport 4.1.x:

% ruby -e '
gem "activesupport", "4.1.16"
require "active_support/time"
10.days'
/PREFIX/lib/ruby/gems/2.3.0/gems/activesupport-4.1.16/lib/active_support/core_ext/numeric/time.rb:50:in `*': ActiveSupport::Duration can't be coerced into Fixnum (TypeError)
        from /PREFIX/lib/ruby/gems/2.3.0/gems/activesupport-4.1.16/lib/active_support/core_ext/numeric/time.rb:50:in `days'
        from -e:4:in `<main>'

This could be fixed by defining respond_to_missing? as follows:

class ActiveSupport::Duration
  def respond_to_missing?(name, private = false)
    value.respond_to?(name, false)
  end
end

However, some gems such as Devise call the time unit methods while they are required, and it is not always easy for a user application to monkey-patch this if it uses Bundler to require all gems at once.

Now, it has already been announced that Rails 4.1 is going to see EOL in no distant future, but considering Rails 4.1.16 was released pretty recently, I think we should probably cooperate with the Rails team and make sure the upcoming 2.3.2 release will not break existing Rails 4.1 applications without prior notice.

Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago

r55500 breaks 1.days, while it fixed Marshal.dump with object which respond to marshal_dump with method_missing.
Even though it's obviously a bug-fix, introduce more critical issue as for activesupport-4.1.x.
I think I should revert r55869 for a while.

Updated by knu (Akinori MUSHA) about 3 years ago

So, can we revert the backport?

This issue spotted a feature bug and it got fixed in trunk, but the original problem (Marshall.dump(1.day) does not work with ruby 2.3+ and activesupport 4.1.x) was not solved by the fix, causing a more serious problem in terms of backward compatibility.

As was confirmed it works with activesupport 4.2+, so I think it will be fine if Ruby 2.4 and later do not run Rails 4.1, since it's almost certain that the 2.4 series will start off after 4.1 effectively reaches its EOL.

#10

Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • Status changed from Open to Closed

Applied in changeset ruby_2_3|r56022.


  • vm_method.c: revert r55869. it breaks Integer#days with
    ActiveSupport-4.1.x. [ruby-core:76949] [Bug #12353]

  • test/ruby/test_marshal.rb: ditto.

Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • Backport changed from 2.1: DONTNEED, 2.2: DONTNEED, 2.3: DONE to 2.1: DONTNEED, 2.2: DONTNEED, 2.3: REQUIRED
  • Status changed from Closed to Open

I reverted r55869 partially at r56022.
And I keep opend this ticket because there's still problem with activesupport 4.1.x.

#12

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

  • Status changed from Open to Closed

Also available in: Atom PDF