Bug #7304

Random test failures around test_autoclose_true_closed_by_finalizer

Added by Luis Lavena over 1 year ago. Updated over 1 year ago.

[ruby-core:49044]
Status:Closed
Priority:High
Assignee:Hiroshi Shirosaki
Category:core
Target version:2.0.0
ruby -v:ruby 2.0.0dev (2012-11-07 trunk 37538) [i386-mingw32] Backport:

Description

=begin
Hello,

Over the past few days I've seen on and off failures on RubyInstaller CI related to (({testautoclosetrueclosedby_finalizer})):

http://ci.rubyinstaller.org/job/ruby-trunk-x86-test-all/265/console

1) Error:
testautoclosetrueclosedbyfinalizer(TestIO):
NoMethodError: undefined method close' for 2012-11-07 04:43:41 -0300:WeakRef
C:/Users/Worker/Jenkins/workspace/ruby-trunk-x86-build/test/ruby/test_io.rb:1611:in
test
autoclosetrueclosedbyfinalizer'

This seems to happen when the system is under heavy load (because is running other jobs in parallel).

This might be a hint of something not working properly under heavy load, perhaps the GC in effect.

I was unable to produce the same failure on x64-mingw32, and haven't tried yet OSX or Linux.

Any ideas?
=end

0001-Fix-WeakRef-finalize.patch Magnifier (3.42 KB) Hiroshi Shirosaki, 11/09/2012 06:33 PM

Associated revisions

Revision 37826
Added by shirosaki over 1 year ago

Fix finalize of WeakRef

  • gc.c (wmapfinalfunc): remove WeakRef object reference from the
    array.

  • gc.c (wmapfinalize): remove recycled object references from weak
    map hash properly. How to get object reference from object id was
    wrong. st
    delete() doesn't work properly if key and value arguments
    are same. The key of obj2wmap is referenced object and the value of
    obj2wmap is WeakRef array.

  • gc.c (wmap_aset): obj2wmap should contain WeakRef array in the
    definition.

  • test/testweakref.rb
    (TestWeakRef#test
    notreferencedifferent_object): add a test for
    above.
    [Bug #7304]

Revision 37827
Added by shirosaki over 1 year ago

wmap_finalize: refactoring to rename variables

  • gc.c (wmapfinalfunc): rename variables to clarify the meaning.
    In wmap2obj the key is WeakRef and the value is referenced object.
    In obj2wmap the key is referenced object and the value is an array
    of WeakRef.

  • gc.c (wmap_finalize): ditto.
    [Bug #7304]

Revision 37834
Added by shirosaki over 1 year ago

Fix WeakRef finalize

  • array.c (rbarydeletesameobj): new function for WeakRef.
    This deletes same objects as item argument in the array.

  • internal.h (rbarydeletesameobj): add a declaration.

  • gc.c (wmapfinalfunc): remove WeakRef object reference from the
    array. rbarydelete() is not usable because it uses rb_equal() to
    compare object references.

  • gc.c (wmapfinalize): remove recycled object references from weak
    map hash properly. How to get object reference from object id was
    wrong. st
    delete() doesn't work properly if key and value arguments
    are same. The key of obj2wmap is referenced object and the value of
    obj2wmap is WeakRef array.

  • gc.c (wmap_aset): obj2wmap should contain WeakRef array in the
    definition.

  • test/testweakref.rb
    (TestWeakRef#test
    notreferencedifferentobject,
    TestWeakRef#test
    weakref_finalize): add tests for above.
    [Bug #7304]

Revision 37835
Added by shirosaki over 1 year ago

gc.c: refactoring to rename variables

  • gc.c (wmapfinalfunc): rename variables to clarify the meaning.
    In wmap2obj the key is WeakRef and the value is referenced object.
    In obj2wmap the key is referenced object and the value is an array
    of WeakRef.

  • gc.c (wmap_finalize): ditto.
    [Bug #7304]

History

#1 Updated by Luis Lavena over 1 year ago

  • Status changed from Open to Assigned
  • ruby -v changed from ruby -v: ruby 2.0.0dev (2012-11-07 trunk 37538) [i386-mingw32] to ruby 2.0.0dev (2012-11-07 trunk 37538) [i386-mingw32]

#2 Updated by Hiroshi Shirosaki over 1 year ago

=begin

I cannot reproduce above error. However, Bug #4168 and #5350 seem not solved.
I got NoMethodError by the following script. WeakRef object has reference to different object from originally associated.

% cat test_weakref.rb
require "weakref"

class Foo
def foo; end
end

a = []
1000.times do
a << WeakRef.new(Foo.new)
end

a.each do |x|
begin
x.foo
rescue WeakRef::RefError
p :referr
end
end

% ruby -v testweakref.rb
ruby 2.0.0dev (2012-11-08 trunk 37558) [i686-linux]
test
weakref.rb:14:in block in <main>': undefined methodfoo' for [70032780]:WeakRef (NoMethodError)
from test_weakref.rb:12:in each'
from test_weakref.rb:12:in
'
=end

#3 Updated by Hiroshi Shirosaki over 1 year ago

After some investigation, I found WeakRef finalize code appears wrong.
When finalize, object references were not removed from weakmap hash properly.

I attached a patch. I tested it with ruby 2.0.0dev (2012-11-09 trunk 37558) [i686-linux].

#4 Updated by Luis Lavena over 1 year ago

  • Assignee changed from Koichi Sasada to Narihiro Nakamura
  • Priority changed from Normal to High

=begin
Thank you Shirosaki-san,

Applying the patch, it fixes the WeakRef issues.

ruby -v: ruby 2.0.0dev (2012-11-10 trunk 37612) [i386-mingw32]
3 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Reassigning to Narihiro Nakamura, as the changes seems to be GC-related?

=end

#5 Updated by Nobuyoshi Nakada over 1 year ago

  • Category changed from test to core
  • Assignee changed from Narihiro Nakamura to Hiroshi Shirosaki

Would you split the patch into refactor by renaming and the fix?

#6 Updated by Hiroshi Shirosaki over 1 year ago

  • Assignee changed from Hiroshi Shirosaki to Nobuyoshi Nakada

I've splited the patch into two commits and pushed it to github.

Could you check it? Thank you.
https://github.com/shirosaki/ruby/compare/trunk...weakref

#7 Updated by Luis Lavena over 1 year ago

Hello Nobu,

As pointed by Shirosaki-san, the two commits are now split.

Can we apply those changes to trunk? Who should be assigned to final approval?

Thank you

#8 Updated by Nobuyoshi Nakada over 1 year ago

go ahead

#9 Updated by Luis Lavena over 1 year ago

  • Assignee changed from Nobuyoshi Nakada to Hiroshi Shirosaki

Thank you Nobu,

Hiroshi, Nobu give you green light to commit the changes from the branch.

Thank you both!

#10 Updated by Anonymous over 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37826.
Luis, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Fix finalize of WeakRef

  • gc.c (wmapfinalfunc): remove WeakRef object reference from the
    array.

  • gc.c (wmapfinalize): remove recycled object references from weak
    map hash properly. How to get object reference from object id was
    wrong. st
    delete() doesn't work properly if key and value arguments
    are same. The key of obj2wmap is referenced object and the value of
    obj2wmap is WeakRef array.

  • gc.c (wmap_aset): obj2wmap should contain WeakRef array in the
    definition.

  • test/testweakref.rb
    (TestWeakRef#test
    notreferencedifferent_object): add a test for
    above.
    [Bug #7304]

#11 Updated by Hiroshi Shirosaki over 1 year ago

  • Status changed from Closed to Assigned
  • % Done changed from 100 to 0

This fix causes segv, which was pointed out at r37831. Thank you, naruse-san.
I found rbarydelete(ary, obj) is not usable when doing WeakRef finalize because rbarydelete() calls rb_equal() against GC'ed WeakRef object.
Instead just comparing VALUE by '==' seems good for this case. I'll fix later.

#12 Updated by Anonymous over 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37834.
Luis, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Fix WeakRef finalize

  • array.c (rbarydeletesameobj): new function for WeakRef.
    This deletes same objects as item argument in the array.

  • internal.h (rbarydeletesameobj): add a declaration.

  • gc.c (wmapfinalfunc): remove WeakRef object reference from the
    array. rbarydelete() is not usable because it uses rb_equal() to
    compare object references.

  • gc.c (wmapfinalize): remove recycled object references from weak
    map hash properly. How to get object reference from object id was
    wrong. st
    delete() doesn't work properly if key and value arguments
    are same. The key of obj2wmap is referenced object and the value of
    obj2wmap is WeakRef array.

  • gc.c (wmap_aset): obj2wmap should contain WeakRef array in the
    definition.

  • test/testweakref.rb
    (TestWeakRef#test
    notreferencedifferentobject,
    TestWeakRef#test
    weakref_finalize): add tests for above.
    [Bug #7304]

Also available in: Atom PDF