-
Notifications
You must be signed in to change notification settings - Fork 605
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
Closed
+2,187
−739
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…; remove debug prints
Conflicts: kernel/common/io.rb
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++.
@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! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.