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.

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

Dumping Hash with default_proc that was reset #4302

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

Comments

Projects
None yet
4 participants
@kirs
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

This comment has been minimized.

Show comment
Hide comment
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 16, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 16, 2016

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 16, 2016

Member

@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!

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 16, 2016

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 18, 2016

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 18, 2016

Member
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 :)

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 18, 2016

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 18, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 18, 2016

Member

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

Member

enebo commented Nov 18, 2016

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 19, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 19, 2016

Member

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

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

Fix marshaling Hash with default_proc set to nil
When we set `default_proc` to nil, we also have to update the internal
flag.

Fixes
jruby#4302
@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 20, 2016

Contributor

Made my first PR to JRuby 🎉

#4321

Contributor

kirs commented Nov 20, 2016

Made my first PR to JRuby 🎉

#4321

@enebo enebo closed this in #4321 Nov 21, 2016

@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