Skip to content
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

Merge ffi-io branch into master #3512

Closed
wants to merge 156 commits into from
Closed

Merge ffi-io branch into master #3512

wants to merge 156 commits into from

Conversation

chuckremes
Copy link
Member

Here's a pull request to start discussing merging this branch into master. Let's talk code style, refactoring, specs, or whatever else is relevant.

This is really weird. The code calls #find_type in the scope
of the enclosing_module, but there could be a conflict with
that method name, numbers of args it takes, its purpose, etc.
So a failure here should be rescued and allow the followup
code to call #find_type on the FFI class.

This was discovered when the ffi-io branch failed to build
a native extension. The mkmf.rb gem was doing some logging
during the build which caused some code in IO::Select to
call IO::Select.class_eval on code to instantiate the
Timeval_t struct class. Creating a struct calls #find_type
but it was calling it from the scope of mkmf.rb which had
its own (completely different!) method named #find_type
defined. It threw an ArgumentError because the arity didn't
match, but it lead me to this logic error and fix.
During a call to IO.reopen a file descriptor was dup2'ed. Its
ftype changed from "file" to "fifo" and as we know a fifo cannot
seek. So we were blowing up when trying to set the offset ivar.
By only allowing seek on "file" FDs, we'll avoid the bug.
As far as I can tell, there is no #buffer_empty? public method on
the IO class. We had one to test this behavior. Gone!
We had a failing spec where a thread blocked on read was
supposed to raise IOError when *another thread* closed
the IO. However, it didn't work. Turns out that the
low-level system read function blocks forever so the
closed IO/file descriptor didn't raise as expected.

The original C code got around this OS behavior by setting
special flags and sending signals to the blocked thread
to wake it up. This mechanism was only exposed to the
bytecode VM and wasn't available to the Ruby runtime so
I had to go this other direction. It's less than ideal
but if the long-term plan is to utilize libuv for
Rubinius IO then this is a good enough fix.
I don't really like this solution very much. Ideally the
Socket classes would set @fd directly in their #initialize
methods via a call to FileDescriptor.choose_type(fd). Having
a +fd+ accessor on the IO class was intended only for
debugging purposes.

Let's go with this patch for now. If the larger refactoring
of the IO class to use a private IO::FileDescriptor class
is accepted then we can go back and change the Socket methods
to conform better and remove this accessor.
Fixed a conflict in io.rb with #write_nonblock and a
conflict in spec/ruby/core/io/read_nonblock_spec.rb.
The IO finalizer is tricky since it also has to detect possible
modifications from C API calls. There will likely need to be
some rework done on the C API code itself to make this easier
to manage from the Ruby side since we are trying to get as much
as possible out of C/C++.
@chuckremes
Copy link
Member Author

@brixen did a bunch of work to merge the codedb branch into the ffi-io branch, so we'll use that going forward. This PR will be closed and I'll open one for merging the codedb-ffi-io branch into master. That long branch name will encourage us to merge it quickly to save precious keystrokes!

@chuckremes chuckremes closed this Feb 7, 2016
@chuckremes chuckremes deleted the ffi-io branch February 8, 2016 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants