org.jruby.ext.pathname.RubyPathname class doesn't properly support instance_variable_set method #1460

Closed
pablito opened this Issue Jan 29, 2014 · 5 comments

Projects

None yet

3 participants

@pablito
pablito commented Jan 29, 2014

I found this nasty behaviour of Pathname object (rel. 1.7.5 and up)

paoloa@setix-aqua:~$ jruby -S irb
jruby-1.7.10 :001 > require 'pathname'
 => true 
jruby-1.7.10 :002 > p=Pathname.new('test')
 => <Pathname:test> 
jruby-1.7.10 :003 > p.instance_variable_set(:@path, 'foo')
 => "foo" 
jruby-1.7.10 :004 > p.to_path
 => "test" 
jruby-1.7.10 :005 > p.instance_variable_get(:@path)
 => "foo" 

this happens because the java implementation of Pathname (class org.jruby.ext.pathname.RubyPathname) stores the path value used by all its methods into a private field and only synchronizes it with @path instance var at initialization time.

@JRubyClass(name = "Pathname")
public class RubyPathname extends RubyObject {
    private RubyString path;
...

@JRubyMethod
public IRubyObject initialize(ThreadContext context, IRubyObject path) {
    ...
    [snipped]
    ...
    // TODO: remove (either direct bridge to field or all native)
    setInstanceVariable("@path", this.path);
    return this;
}


Since @path is never used internally, changing its value has no effect on object behaviour. A byproduct of this is that YAML deserialization always fails.

paoloa@setix-aqua:~$ jruby -S irb
jruby-1.7.10 :001 > require 'yaml'
 => true 
jruby-1.7.10 :002 > require 'pathname'
 => true 
jruby-1.7.10 :003 > p = Pathname.new('test')
 => <Pathname:test> 
jruby-1.7.10 :004 > p.to_yaml
 => "--- !ruby/object:Pathname\npath: test\n" 
jruby-1.7.10 :006 > deserialized = YAML.load(p.to_yaml)
 => <Pathname:null> 
jruby-1.7.10 :008 > deserialized.to_path
 => nil 
@eregon eregon was assigned by BanzaiMan Feb 9, 2014
@pablito
pablito commented Feb 10, 2014

Hello, I looked again at this issue and noticed that on master branch pathname.rb has been reverted to 1.7.4 (i.e. pure ruby with no java implementation).
I don't know if this is wanted or is a regression.

@eregon
Member
eregon commented Feb 11, 2014

Probably unintentional (the commit is 6d5ddac by @headius).
We should indeed synchronize on ivar set, but I do not know how to do that unfortunately.
The alternative would be to always use only the ruby ivar, in which case I think there is little interest in having a native implementation.

BTW, making YAML use the proper constructor would be good anyway, I am starting to feel #allocate then #instance_variable_set is a bad pattern since it avoids any validation.

@pablito
pablito commented Feb 11, 2014

I had a peek at the MRI implementation, it uses a ruby ivar.

the only way of keeping the ivar synchronized that I can think of is to define a private setter for path which also sets the ruby ivar and never access path directly elsewhere in the code, but it is obviously error prone and very similar to directly using a ruby ivar.
private void set_path(RubyString _path){ this.path = _path; setInstanceVariable("@path", this.path); }

#instance_variable_set is public, so it's not only a case of YAML using a bad pattern

@eregon
Member
eregon commented Mar 1, 2014

@headius Should I always and only use the ivar?
Overriding Object#instance_variable_set is more than probably a very bad idea, is it not?

@eregon eregon added a commit that closed this issue Mar 10, 2014
@eregon eregon Use an actual Ruby instance variable in Pathname
* Fixes #1460
* So we are sure to be fully compatible with MRI
* #instance_variable_set must be reflected and redefining it on Pathname seems
  a very bad idea (even though Pathname is immutable without #instance_variable_set)
* We need anyway to add the instance variable for subclasses which access @path
  directly
* An alternative would be to define a YAML deserialization hook
  but we may face other compatibility issues
dec3a74
@eregon eregon closed this in dec3a74 Mar 10, 2014
@eregon
Member
eregon commented Mar 10, 2014

Should be fixed, sorry for the long delay.

@eregon eregon added a commit that referenced this issue Mar 10, 2014
@eregon eregon Use an actual Ruby instance variable in Pathname
* Fixes #1460
* So we are sure to be fully compatible with MRI
* #instance_variable_set must be reflected and redefining it on Pathname seems
  a very bad idea (even though Pathname is immutable without #instance_variable_set)
* We need anyway to add the instance variable for subclasses which access @path
  directly
* An alternative would be to define a YAML deserialization hook
  but we may face other compatibility issues
f767b9c
@enebo enebo added this to the JRuby 1.7.12 milestone Mar 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment