diff --git a/ext/ffi_c/Call.c b/ext/ffi_c/Call.c index 0b9aceb87..d0a712250 100644 --- a/ext/ffi_c/Call.c +++ b/ext/ffi_c/Call.c @@ -116,7 +116,7 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes, Type* paramType = paramTypes[i]; int type; - + if (unlikely(paramType->nativeType == NATIVE_MAPPED)) { VALUE values[] = { argv[argidx], Qnil }; argv[argidx] = rb_funcall2(((MappedType *) paramType)->rbConverter, id_to_native, 2, values); @@ -297,8 +297,8 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes, case NATIVE_STRING: if (type == T_NIL) { - param->ptr = NULL; - + param->ptr = NULL; + } else { if (rb_safe_level() >= 1 && OBJ_TAINTED(argv[argidx])) { rb_raise(rb_eSecurityError, "Unsafe string parameter"); @@ -345,9 +345,13 @@ static void * call_blocking_function(void* data) { rbffi_blocking_call_t* b = (rbffi_blocking_call_t *) data; +#ifndef HAVE_RUBY_THREAD_HAS_GVL_P b->frame->has_gvl = false; +#endif ffi_call(&b->cif, FFI_FN(b->function), b->retval, b->ffiValues); +#ifndef HAVE_RUBY_THREAD_HAS_GVL_P b->frame->has_gvl = true; +#endif return NULL; } @@ -376,9 +380,9 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo) FFIStorage* params; VALUE rbReturnValue; rbffi_frame_t frame = { 0 }; - + retval = alloca(MAX(fnInfo->ffi_cif.rtype->size, FFI_SIZEOF_ARG)); - + if (unlikely(fnInfo->blocking)) { rbffi_blocking_call_t* bc; @@ -408,7 +412,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo) fnInfo->parameterCount, fnInfo->parameterTypes, params, ffiValues, fnInfo->callbackParameters, fnInfo->callbackCount, fnInfo->rbEnums); - rbffi_frame_push(&frame); + rbffi_frame_push(&frame); rb_rescue2(rbffi_do_blocking_call, (VALUE) bc, rbffi_save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0); rbffi_frame_pop(&frame); @@ -419,7 +423,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo) xfree(bc->retval); xfree(bc); #endif - + } else { ffiValues = ALLOCA_N(void *, fnInfo->parameterCount); @@ -436,7 +440,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo) if (unlikely(!fnInfo->ignoreErrno)) { rbffi_save_errno(); - } + } if (RTEST(frame.exc) && frame.exc != Qnil) { rb_exc_raise(frame.exc); @@ -444,7 +448,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo) RB_GC_GUARD(rbReturnValue) = rbffi_NativeValue_ToRuby(fnInfo->returnType, fnInfo->rbReturnType, retval); RB_GC_GUARD(fnInfo->rbReturnType); - + return rbReturnValue; } @@ -461,7 +465,7 @@ getPointer(VALUE value, int type) return memory != NULL ? memory->address : NULL; } else if (type == T_STRING) { - + return StringValuePtr(value); } else if (type == T_NIL) { diff --git a/ext/ffi_c/Call.h b/ext/ffi_c/Call.h index 56bdd61a6..dc3f9825e 100644 --- a/ext/ffi_c/Call.h +++ b/ext/ffi_c/Call.h @@ -48,7 +48,7 @@ extern "C" { #if (defined(__i386__) || defined(__x86_64__)) && !(defined(_WIN32) || defined(__WIN32__)) # define BYPASS_FFI 1 #endif - + typedef union { #ifdef USE_RAW signed int s8, s16, s32; @@ -70,7 +70,7 @@ typedef union { double f64; long double ld; } FFIStorage; - + extern void rbffi_Call_Init(VALUE moduleFFI); extern void rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes, diff --git a/ext/ffi_c/Function.c b/ext/ffi_c/Function.c index 9e861ff9e..7bbd30461 100644 --- a/ext/ffi_c/Function.c +++ b/ext/ffi_c/Function.c @@ -105,6 +105,27 @@ static VALUE async_cb_call(void *); extern void *rb_thread_call_with_gvl(void *(*func)(void *), void *data1); #endif +#ifdef HAVE_RUBY_THREAD_HAS_GVL_P +extern int ruby_thread_has_gvl_p(void); +#define rbffi_thread_has_gvl_p(frame) ruby_thread_has_gvl_p() +#else +static int rbffi_thread_has_gvl_p(rbffi_frame_t *frame) +{ + return frame != NULL && frame->has_gvl; +} +#endif + +#ifdef HAVE_RUBY_NATIVE_THREAD_P +extern int ruby_native_thread_p(void); +#define rbffi_native_thread_p(frame) ruby_native_thread_p() +#else +static int rbffi_native_thread_p(rbffi_frame_t *frame) +{ + return frame != NULL; +} +#endif + + VALUE rbffi_FunctionClass = Qnil; #if defined(DEFER_ASYNC_CALLBACK) @@ -474,13 +495,13 @@ callback_invoke(ffi_cif* cif, void* retval, void** parameters, void* user_data) cb.frame = rbffi_frame_current(); if (cb.frame != NULL) cb.frame->exc = Qnil; - if (cb.frame != NULL && cb.frame->has_gvl) { - callback_with_gvl(&cb); -#if defined(HAVE_RB_THREAD_CALL_WITH_GVL) - } else if (cb.frame != NULL) { + if (rbffi_native_thread_p(cb.frame)) { + if(rbffi_thread_has_gvl_p(cb.frame)) { + callback_with_gvl(&cb); + } else { rb_thread_call_with_gvl(callback_with_gvl, &cb); -#endif + } #if defined(DEFER_ASYNC_CALLBACK) && !defined(_WIN32) } else { bool empty = false; diff --git a/ext/ffi_c/Thread.c b/ext/ffi_c/Thread.c index e85ea40b2..f59c2b8dd 100644 --- a/ext/ffi_c/Thread.c +++ b/ext/ffi_c/Thread.c @@ -70,13 +70,15 @@ rbffi_frame_current(void) #endif } -void +void rbffi_frame_push(rbffi_frame_t* frame) { memset(frame, 0, sizeof(*frame)); +#ifndef HAVE_RUBY_THREAD_HAS_GVL_P frame->has_gvl = true; +#endif frame->exc = Qnil; - + #ifdef _WIN32 frame->prev = TlsGetValue(frame_thread_key); TlsSetValue(frame_thread_key, frame); @@ -87,7 +89,7 @@ rbffi_frame_push(rbffi_frame_t* frame) #endif } -void +void rbffi_frame_pop(rbffi_frame_t* frame) { #ifdef _WIN32 @@ -118,13 +120,13 @@ rbffi_blocking_thread(void* args) struct BlockingThread* thr = (struct BlockingThread *) args; char c = 1; VALUE retval; - + retval = (*thr->fn)(thr->data); - + pthread_testcancel(); thr->retval = retval; - + write(thr->wrfd, &c, sizeof(c)); return NULL; @@ -135,7 +137,7 @@ wait_for_thread(void *data) { struct BlockingThread* thr = (struct BlockingThread *) data; char c; - + if (read(thr->rdfd, &c, 1) < 1) { rb_thread_wait_fd(thr->rdfd); while (read(thr->rdfd, &c, 1) < 1 && rb_io_wait_readable(thr->rdfd) == Qtrue) { @@ -166,7 +168,7 @@ rbffi_thread_blocking_region(VALUE (*func)(void *), void *data1, void (*ubf)(voi struct BlockingThread* thr; int fd[2]; VALUE exc; - + if (pipe(fd) < 0) { rb_raise(rb_eSystemCallError, "pipe(2) failed"); return Qnil; @@ -192,7 +194,7 @@ rbffi_thread_blocking_region(VALUE (*func)(void *), void *data1, void (*ubf)(voi exc = rb_rescue2(wait_for_thread, (VALUE) thr, cleanup_blocking_thread, (VALUE) thr, rb_eException); - + pthread_join(thr->tid, NULL); close(fd[1]); close(fd[0]); @@ -348,6 +350,6 @@ rbffi_Thread_Init(VALUE moduleFFI) #ifdef _WIN32 frame_thread_key = TlsAlloc(); #else - pthread_key_create(&thread_data_key, thread_data_free); + pthread_key_create(&thread_data_key, thread_data_free); #endif } diff --git a/ext/ffi_c/Thread.h b/ext/ffi_c/Thread.h index c51a5a93e..b660b36e6 100644 --- a/ext/ffi_c/Thread.h +++ b/ext/ffi_c/Thread.h @@ -66,7 +66,9 @@ typedef struct rbffi_frame { struct thread_data* td; #endif struct rbffi_frame* prev; +#ifndef HAVE_RUBY_THREAD_HAS_GVL_P bool has_gvl; +#endif VALUE exc; } rbffi_frame_t; diff --git a/ext/ffi_c/extconf.rb b/ext/ffi_c/extconf.rb index 45ab9770f..092736644 100644 --- a/ext/ffi_c/extconf.rb +++ b/ext/ffi_c/extconf.rb @@ -30,6 +30,11 @@ have_func('rb_thread_blocking_region') have_func('rb_thread_call_with_gvl') have_func('rb_thread_call_without_gvl') + have_func('ruby_native_thread_p') + if RbConfig::CONFIG['host_os'].downcase !~ /darwin/ || RUBY_VERSION >= "2.3.0" + # On OSX ruby_thread_has_gvl_p is detected but fails at runtime for ruby < 2.3.0 + have_func('ruby_thread_has_gvl_p') + end if libffi_ok have_func('ffi_prep_cif_var') @@ -43,7 +48,7 @@ $defs << "-DFFI_BUILDING" if RbConfig::CONFIG['host_os'] =~ /mswin/ # for compatibility with newer libffi create_header - + $LOCAL_LIBS << " ./libffi/.libs/libffi_convenience.lib" if !libffi_ok && RbConfig::CONFIG['host_os'] =~ /mswin/ create_makefile("ffi_c") @@ -63,7 +68,7 @@ end end end - + else File.open("Makefile", "w") do |mf| mf.puts "# Dummy makefile for non-mri rubies" diff --git a/spec/ffi/callback_spec.rb b/spec/ffi/callback_spec.rb index 38c263612..9d66e2aa7 100644 --- a/spec/ffi/callback_spec.rb +++ b/spec/ffi/callback_spec.rb @@ -772,3 +772,98 @@ module LibTestStdcall end end end + +describe "Callback interop" do + require 'fiddle' + require 'fiddle/import' + require 'timeout' + + module LibTestFFI + extend FFI::Library + ffi_lib TestLibrary::PATH + attach_function :testCallbackVrV, :testClosureVrV, [ :pointer ], :void + attach_function :testCallbackVrV_blocking, :testClosureVrV, [ :pointer ], :void, blocking: true + end + + module LibTestFiddle + extend Fiddle::Importer + dlload TestLibrary::PATH + extern 'void testClosureVrV(void *fp)' + end + + def assert_callback_in_same_thread_called_once + called = 0 + thread = nil + yield proc { + called += 1 + thread = Thread.current + } + expect(called).to eq(1) + expect(thread).to eq(Thread.current) + end + + it "from ffi to ffi" do + assert_callback_in_same_thread_called_once do |block| + func = FFI::Function.new(:void, [:pointer], &block) + LibTestFFI.testCallbackVrV(FFI::Pointer.new(func.to_i)) + end + end + + it "from ffi to ffi with blocking:true" do + assert_callback_in_same_thread_called_once do |block| + func = FFI::Function.new(:void, [:pointer], &block) + LibTestFFI.testCallbackVrV_blocking(FFI::Pointer.new(func.to_i)) + end + end + + # https://github.com/ffi/ffi/issues/527 + if RUBY_VERSION.split('.').map(&:to_i).pack("C*") >= [2,3,0].pack("C*") || RUBY_PLATFORM =~ /java/ + it "from fiddle to ffi" do + assert_callback_in_same_thread_called_once do |block| + func = FFI::Function.new(:void, [:pointer], &block) + LibTestFiddle.testClosureVrV(Fiddle::Pointer[func.to_i]) + end + end + end + + it "from ffi to fiddle" do + assert_callback_in_same_thread_called_once do |block| + func = LibTestFiddle.bind_function(:cbVrV, Fiddle::TYPE_VOID, [], &block) + LibTestFFI.testCallbackVrV(FFI::Pointer.new(func.to_i)) + end + end + + it "from ffi to fiddle with blocking:true" do + assert_callback_in_same_thread_called_once do |block| + func = LibTestFiddle.bind_function(:cbVrV, Fiddle::TYPE_VOID, [], &block) + LibTestFFI.testCallbackVrV_blocking(FFI::Pointer.new(func.to_i)) + end + end + + it "from fiddle to fiddle" do + assert_callback_in_same_thread_called_once do |block| + func = LibTestFiddle.bind_function(:cbVrV, Fiddle::TYPE_VOID, [], &block) + LibTestFiddle.testClosureVrV(Fiddle::Pointer[func.to_i]) + end + end + + # https://github.com/ffi/ffi/issues/527 + if RUBY_ENGINE == 'ruby' && RUBY_VERSION.split('.').map(&:to_i).pack("C*") >= [2,3,0].pack("C*") + it "C outside ffi call stack does not deadlock [#527]" do + path = File.join(File.dirname(__FILE__), "embed-test/embed-test.rb") + pid = spawn(RbConfig.ruby, "-Ilib", path, { [:out, :err] => "embed-test.log" }) + begin + Timeout.timeout(10){ Process.wait(pid) } + rescue Timeout::Error + Process.kill(9, pid) + raise + else + if $?.exitstatus != 0 + raise "external process failed:\n#{ File.read("embed-test.log") }" + end + end + + expect(File.read("embed-test.log")).to match(/callback called with \["hello", 5, 0\]/) + end + end +end diff --git a/spec/ffi/embed-test/embed-test.rb b/spec/ffi/embed-test/embed-test.rb new file mode 100755 index 000000000..17b4a74ac --- /dev/null +++ b/spec/ffi/embed-test/embed-test.rb @@ -0,0 +1,51 @@ +#!/usr/bin/env ruby +# +# This file is part of ruby-ffi. +# For licensing, see LICENSE.SPECS +# + +# This test specifically avoids calling native code through FFI. +# Instead, the stock extension mechanism is used. The reason is +# that the C extension initializes FFI and then calls a callback +# which deadlocked in earlier FFI versions, see +# https://github.com/ffi/ffi/issues/527 + +require 'rbconfig' +require 'ffi' + +EXT = File.expand_path("ext/embed_test.#{RbConfig::CONFIG['DLEXT']}", File.dirname(__FILE__)) +old = Dir.pwd +Dir.chdir(File.dirname(EXT)) + +nul = File.open(File::NULL) +make = system('type gmake', { :out => nul, :err => nul }) && 'gmake' || 'make' + +# create Makefile +system(RbConfig.ruby, "extconf.rb") + +# compile extension +unless system(make) + raise "Unable to compile \"#{EXT}\"" +end + +Dir.chdir(old) + +puts "load #{EXT}" +require EXT + +module LibWrap + extend FFI::Library + ffi_lib EXT + callback :completion_function, [:string, :long, :uint8], :void + attach_function :do_work, [:pointer, :completion_function], :int + Callback = Proc.new do |buf_ptr, count, code| + puts "callback called with #{[buf_ptr, count, code].inspect}" + nil + end +end + +puts "call do_work()" +LibWrap.do_work("test", LibWrap::Callback) + +puts "call testfunc()" +EmbedTest::testfunc diff --git a/spec/ffi/embed-test/ext/embed.c b/spec/ffi/embed-test/ext/embed.c new file mode 100644 index 000000000..11023f965 --- /dev/null +++ b/spec/ffi/embed-test/ext/embed.c @@ -0,0 +1,58 @@ +/* + * Copyright (C) 2017 Thomas Martitz + * Copyright (C) 2008-2017, Ruby FFI project contributors + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * * Neither the name of the Ruby FFI project nor the + * names of its contributors may be used to endorse or promote products + * derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include +#include + +typedef void completion_function(const char *buffer, long count, unsigned char code); + +static completion_function *ruby_func; + +#ifdef _WIN32 + __declspec(dllexport) +#endif +int do_work(const char *buffer, completion_function *fn) +{ + ruby_func = fn; + return 0; +} + +static VALUE testfunc(VALUE args); + +void Init_embed_test(void) +{ + VALUE mod = rb_define_module("EmbedTest"); + rb_define_module_function(mod, "testfunc", testfunc, 0); +} + +static VALUE testfunc(VALUE self) +{ + printf("testfunc() called\n"); + ruby_func("hello", 5, 0); + return Qnil; +} diff --git a/spec/ffi/embed-test/ext/extconf.rb b/spec/ffi/embed-test/ext/extconf.rb new file mode 100644 index 000000000..6327e0ac5 --- /dev/null +++ b/spec/ffi/embed-test/ext/extconf.rb @@ -0,0 +1,11 @@ +#!/usr/bin/env ruby +# +# This file is part of ruby-ffi. +# For licensing, see LICENSE.SPECS +# + +if !defined?(RUBY_ENGINE) || RUBY_ENGINE == 'ruby' || RUBY_ENGINE == 'rbx' + require "mkmf" + + create_makefile("embed_test") +end