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.

Sign up for GitHub

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

Jump to bottom

Dumping Hash with default_proc that was reset #4302

Closed
kirs opened this issue Nov 16, 2016 · 13 comments
Closed

Dumping Hash with default_proc that was reset #4302

kirs opened this issue Nov 16, 2016 · 13 comments
Labels
core
Milestone
JRuby 9.1.7.0

Comments

@kirs
Copy link
Contributor

kirs commented Nov 16, 2016 

# default_proc.rb
hash = Hash.new { |k| "default value" }
hash.default_proc = nil
Marshal.dump(hash)

$ jruby default_proc.rb
TypeError: can't dump hash with default proc
    dump at org/jruby/RubyMarshal.java:102
  <main> at default_proc.rb:3

Running on jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b14 +jit [darwin-x86_64]

Expected Behavior

JRuby should be able to dump a hash that was initialized with default_proc that was later set to nil.

Actual Behavior

JRuby currently ignores default_proc set to nil and gives an error that a hash with default_proc cannot be dumped.

@headius @guilleiguaran
@guilleiguaran
Copy link
Member

I guess this is related to this failure:
https://github.com/jruby/jruby/blob/master/spec/tags/ruby/core/hash/default_proc_tags.txt

@headius
Copy link
Member

headius commented Nov 16, 2016

@guilleiguaran Very likely, as well as some failures @enebo was working on in ActiveRecord!

@enebo enebo added this to the JRuby 9.1.7.0 milestone Nov 16, 2016
@kirs
Copy link
Contributor Author

kirs commented Nov 16, 2016

@enebo please let me know if you already worked on it. If not, I'll be happy to submit a PR

@enebo
Copy link
Member

enebo commented Nov 16, 2016

@guilleiguaran OMGZ you rock. I have been trying to figure out what was supplying a default Hash to marshal in a local unit test in arjdbc and did not realize 2.3 allowed this. So it is in fact a JRuby bug and not a arjdbc one. This will save me time since I will now look in the right place!

@enebo
Copy link
Member

enebo commented Nov 16, 2016

@guilleiguaran wow github is not working great this morning. Yeah if you can figure this out then go for it. I would figure out the commit in MRI and work from there.

@kirs
Copy link
Contributor Author

kirs commented Nov 18, 2016

So I cloned the repo, installed dependencies and tried to run test related to the Hash class:

$ git checkout master
$ bin/jruby test/mri/runner.rb test/mri/ruby/test_hash.rb

But many of them failed:

206 tests, 1413 assertions, 12 failures, 10 errors, 1 skips

It is supposed to be stable and green atm? Or is there some specific branch I should look at?

@enebo
Copy link
Member

enebo commented Nov 18, 2016 

jruby -r ./test/mri_test_env.rb test/mri/runner.rb  -v test/mri/ruby/test_hash.rb

We exclude some tests. Some other sensible like tests involving callcc (which we cannot support) and others we look to people like @kirs to help us :)

@enebo
Copy link
Member

enebo commented Nov 18, 2016

@kirs sorry I should have pointed out we use 'minitest-excludes (2.0.0)' to exclude tests we do not pass so we can be green but know what else we can potentially improve.

@kirs
Copy link
Contributor Author

kirs commented Nov 18, 2016

Thanks! ./test/mri_test_env.rb makes sense to me. I was looking at the docs (https://github.com/jruby/jruby/blame/master/BUILDING.md#L128) that only suggests jruby test/mri/runner.rb test/mri/<path to test>, without the env.

@enebo
Copy link
Member

enebo commented Nov 18, 2016

@kirs update those docs to point out our use of minitest-excludes.

@headius
Copy link
Member

headius commented Nov 19, 2016

The proper way to run with excludes is described in BUILDING.md a few lines below the initial runner line: https://github.com/jruby/jruby/blame/master/BUILDING.md#L140

@enebo
Copy link
Member

enebo commented Nov 19, 2016

My comment was confusing but I did change the line above that. It sounded like I wanted him to change it but I did...hmm maybe I should revert that

kirs added a commit to kirs/jruby that referenced this issue Nov 20, 2016
@kirs
Fix marshaling Hash with default_proc set to nil
f36f607 
When we set `default_proc` to nil, we also have to update the internal
flag.

Fixes
jruby#4302
@kirs kirs mentioned this issue Nov 20, 2016
Merged
@kirs
Copy link
Contributor Author

kirs commented Nov 20, 2016

Made my first PR to JRuby 🎉

#4321

@enebo enebo added the core label Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
JRuby 9.1.7.0
Development

No branches or pull requests
4 participants
@headius @enebo @guilleiguaran @kirs