Query ruby thread and GVL states instead of relying on our call frame for callbacks #584
Conversation
|
@kugel- FYI |
|
Awesome, thanks |
|
I fixed the failing test on MacOS. |
| 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*") |
There was a problem hiding this comment.
I think you can just compare the strings here as strings.
RUBY_VERSION >= "2.3.0"
Does rbx not support this?
There was a problem hiding this comment.
Since we have seen ruby-2.1.10 and I know you as a very exact character, I thought it's best to compare exactly...
rbx doesn't support fiddle, so that these tests fail. The embed_test seems to work in rbx after some tweeking. May I adjust the tests for compatibility with rbx?
There was a problem hiding this comment.
fair enough...
changing the tests for rbx would depend on the nature of the changes.
There was a problem hiding this comment.
I don't understand your last comment (as a non English speaker). Shall I adjust the tests so that they pass on rbx (although it is no longer tested on travis-ci)?
Anyway I already tested this with JRuby (see 047371b) , so that the update of the testsuite could be merged there someday.
|
yeah. That wasn't super clear even to me on rereading it... Is RBX failing because of incorrect behavior in FFI when running under RBX? |
spec/ffi/embed-test/ext/embed.c
Outdated
| int state = 0; | ||
| VALUE ret; | ||
|
|
||
| rb_eval_string_protect(script, &state); |
There was a problem hiding this comment.
Is RBX failing because of incorrect behavior in FFI when running under RBX?
It's failing because RBX doesn't know rb_eval_string_protect() but only rb_eval_string(). However the state is important to verify that the embedded script worked. So the program has to be changed, so that the success state is transferred in some other way (rb_rescue() or similar).
|
What's holding this back? |
|
Can you rebase this so it runs against the new travis config? |
|
I rebased to current master, but travis-ci tests on MacOS are still pending after 7 hours. |
|
Looks like Travis is still having macOS issues. I’ll run them locally tonight.
|
|
Travis-CI has finished and it is mostly green. |
|
Sorry, looked at the wrong pull request :-( |
larskanis
force-pushed
the
fix-527-v2
branch
5 times, most recently
from
029a5ea to
3f0a33a
Compare
|
Can you rebase this? |
… for callbacks This fixes ffi#527 and other interoperability issues that can happen, when a callback is called without a valid call frame. The change is enabled for ruby-2.3+ only, since it requires the functions ruby_native_thread_p() and ruby_thread_has_gvl_p(). The creation of a call frame is not (yet) removed, because it still carries callback expections around.
This is particular interesting because FFI stores thread local call info on the stack, which is retrieved when a callback is received. This is something that Fiddle doesn't do, but a callback should be handled gracefully nevertheless. The test "from fiddle to ffi" fails without the previous commit. These tests succeed on MRI and on JRuby (provided jruby/jruby#4518 will be merged).
Since the problem is a dead lock (inside pthread_cond_wait, on Linux), the test case has to spawn a separate process which can be killed on failure.
|
OK, rebased and I fixed the issue on Windows. |
Windows doesn't resolve symbols of external DLLs, if called with CURRENT_PROCESS. Instead the DLL has to be named explicit. Add an expectation to the spec while being over it.
This fixes #527 and other interoperability issues that can happen, when a callback is called without a valid call frame.
The change is enabled for ruby-2.3+ only, since it requires the functions ruby_native_thread_p() and ruby_thread_has_gvl_p().
The creation of a call frame is not (yet) removed, because it still carries callback expections around.
This also adds tests for interoperability with Fiddle and embedded ruby use case. Both are affected by the change to the FFI call frame.