From 08b1a6754fb1ebbf340c778e9a015ce0ecc83974 Mon Sep 17 00:00:00 2001 From: Lars Kanis Date: Tue, 2 May 2023 11:28:06 +0200 Subject: Allow private release method for ManagedStruct and AutoPointer FFI users might not want to expose `release` as a public class method, but want to keep it private. Also re-enable GC tests that were disabled as part of #427, but make them opt-in per env var FFI_TEST_GC. Derivations of Releaser are no longer necessary, so that they are removed now. --- .github/workflows/ci.yml | 4 ++-- lib/ffi/autopointer.rb | 24 ++++-------------------- lib/ffi/managedstruct.rb | 2 +- spec/ffi/managed_struct_spec.rb | 2 ++ spec/ffi/pointer_spec.rb | 11 ++++++++--- spec/ffi/spec_helper.rb | 2 +- 6 files changed, 18 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 58fdbd0..371d372 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: env: # work around misconfiguration of libffi on MacOS with homebrew PKG_CONFIG_PATH: ${{ env.PKG_CONFIG_PATH }}:/usr/local/opt/libffi/lib/pkgconfig - - run: bundle exec rake test + - run: bundle exec rake test FFI_TEST_GC=true - run: bundle exec rake types_conf && git --no-pager diff specs: @@ -62,7 +62,7 @@ jobs: - run: bundle exec rake libffi - run: bundle exec rake compile - - run: bundle exec rake test + - run: bundle exec rake test FFI_TEST_GC=true - run: bundle exec rake bench:all if: ${{ matrix.ruby != 'truffleruby-head' && matrix.ruby != 'jruby-head' }} diff --git a/lib/ffi/autopointer.rb b/lib/ffi/autopointer.rb index 6c6333c..4f44171 100644 --- a/lib/ffi/autopointer.rb +++ b/lib/ffi/autopointer.rb @@ -84,13 +84,13 @@ module FFI if not proc.respond_to?(:call) raise RuntimeError.new("proc must be callable") end - CallableReleaser.new(ptr, proc) + Releaser.new(ptr, proc) else - if not self.class.respond_to?(:release) + if not self.class.respond_to?(:release, true) raise RuntimeError.new("no release method defined") end - DefaultReleaser.new(ptr, self.class) + Releaser.new(ptr, self.class.method(:release)) end ObjectSpace.define_finalizer(self, @releaser) @@ -149,23 +149,7 @@ module FFI def call(*args) release(@ptr) if @autorelease && @ptr end - end - - # DefaultReleaser is a {Releaser} used when an {AutoPointer} is defined - # without Proc or Method. In this case, the pointer to release must be of - # a class derived from AutoPointer with a {release} class method. - class DefaultReleaser < Releaser - # @param [Pointer] ptr - # @return [nil] - # Release +ptr+ using the {release} class method of its class. - def release(ptr) - @proc.release(ptr) - end - end - # CallableReleaser is a {Releaser} used when an {AutoPointer} is defined with a - # Proc or a Method. - class CallableReleaser < Releaser # Release +ptr+ by using Proc or Method defined at +ptr+ # {AutoPointer#initialize initialization}. # @@ -182,7 +166,7 @@ module FFI # @return [Type::POINTER] # @raise {RuntimeError} if class does not implement a +#release+ method def self.native_type - if not self.respond_to?(:release) + if not self.respond_to?(:release, true) raise RuntimeError.new("no release method defined for #{self.inspect}") end Type::POINTER diff --git a/lib/ffi/managedstruct.rb b/lib/ffi/managedstruct.rb index b5ec8a3..5d243e5 100644 --- a/lib/ffi/managedstruct.rb +++ b/lib/ffi/managedstruct.rb @@ -75,7 +75,7 @@ module FFI # @overload initialize # A new instance of FFI::ManagedStruct. def initialize(pointer=nil) - raise NoMethodError, "release() not implemented for class #{self}" unless self.class.respond_to? :release + raise NoMethodError, "release() not implemented for class #{self}" unless self.class.respond_to?(:release, true) raise ArgumentError, "Must supply a pointer to memory for the Struct" unless pointer super AutoPointer.new(pointer, self.class.method(:release)) end diff --git a/spec/ffi/managed_struct_spec.rb b/spec/ffi/managed_struct_spec.rb index b4e80bb..6299bdc 100644 --- a/spec/ffi/managed_struct_spec.rb +++ b/spec/ffi/managed_struct_spec.rb @@ -46,6 +46,8 @@ describe "Managed Struct" do def self.release(_ptr) @@count += 1 end + private_class_method :release + def self.wait_gc(count) loop = 5 while loop > 0 && @@count < count diff --git a/spec/ffi/pointer_spec.rb b/spec/ffi/pointer_spec.rb index c8242c7..820e144 100644 --- a/spec/ffi/pointer_spec.rb +++ b/spec/ffi/pointer_spec.rb @@ -257,6 +257,7 @@ describe "AutoPointer" do def self.release @@count += 1 if @@count > 0 end + private_class_method(:release) def self.reset @@count = 0 end @@ -275,10 +276,11 @@ describe "AutoPointer" do end class AutoPointerSubclass < FFI::AutoPointer def self.release(ptr); end + private_class_method(:release) end # see #427 - it "cleanup via default release method", :broken => true do + it "cleanup via default release method", gc_dependent: true do expect(AutoPointerSubclass).to receive(:release).at_least(loop_count-wiggle_room).times AutoPointerTestHelper.reset loop_count.times do @@ -291,7 +293,7 @@ describe "AutoPointer" do end # see #427 - it "cleanup when passed a proc", :broken => true do + it "cleanup when passed a proc", gc_dependent: true do # NOTE: passing a proc is touchy, because it's so easy to create a memory leak. # # specifically, if we made an inline call to @@ -310,7 +312,7 @@ describe "AutoPointer" do end # see #427 - it "cleanup when passed a method", :broken => true do + it "cleanup when passed a method", gc_dependent: true do expect(AutoPointerTestHelper).to receive(:release).at_least(loop_count-wiggle_room).times AutoPointerTestHelper.reset loop_count.times do @@ -327,6 +329,7 @@ describe "AutoPointer" do ffi_lib TestLibrary::PATH class CustomAutoPointer < FFI::AutoPointer def self.release(ptr); end + private_class_method(:release) end attach_function :ptr_from_address, [ FFI::Platform::ADDRESS_SIZE == 32 ? :uint : :ulong_long ], CustomAutoPointer end @@ -349,6 +352,7 @@ describe "AutoPointer" do describe "#autorelease?" do ptr_class = Class.new(FFI::AutoPointer) do def self.release(ptr); end + private_class_method(:release) end it "should be true by default" do @@ -365,6 +369,7 @@ describe "AutoPointer" do describe "#type_size" do ptr_class = Class.new(FFI::AutoPointer) do def self.release(ptr); end + private_class_method(:release) end it "type_size of AutoPointer should match wrapped Pointer" do diff --git a/spec/ffi/spec_helper.rb b/spec/ffi/spec_helper.rb index e9ced3d..a0adbed 100644 --- a/spec/ffi/spec_helper.rb +++ b/spec/ffi/spec_helper.rb @@ -8,7 +8,7 @@ require 'timeout' require 'objspace' RSpec.configure do |c| - c.filter_run_excluding :broken => true + c.filter_run_excluding gc_dependent: true unless ENV['FFI_TEST_GC'] == 'true' end module TestLibrary -- cgit v1.2.1