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

Process#spawn should call #to_io on non-IO file descriptor objects #2809

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Process#spawn should call #to_io on non-IO file descriptor objects #2809

merged 3 commits into from
Dec 19, 2022

Conversation

jcouball
Copy link
Contributor

@jcouball jcouball commented Dec 17, 2022

When an object is passed as the value to a Process#spawn redirection option AND that object isn't a Symbol, Array, Integer, or an IO object AND that object responds to #to_io, then Process#spawn should call #to_io on that object to get the file descriptor.

In TruffleRuby 22.3.0 and head, Process#spawn raises an error in this situation without calling the object's #to_io method.

The MRI implementation has done this since 2.0.0 and JRuby has done this since 9.4.0.0. I have created the project jcouball/convert_to_fd_test to illustrate this. The project contains RSpec tests that test this scenario and uses Github Actions to run these tests on different versions of MRI, JRuby, and TruffleRuby. The results of running these tests on different Rubies can be found here.

I am not able to run the spec in Windows and an hoping those are exercised via the CI build.

@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Dec 17, 2022
@jcouball
Copy link
Contributor Author

I have submitted my Oracle's Contributor Agreement Application and it is being reviewed.

@jcouball jcouball changed the title Spawn should call #to_io on non-IO file descriptor objects Process#spawn should call #to_io on non-IO file descriptor objects Dec 18, 2022
@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Dec 19, 2022
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

I'll rework a bit the specs to use a mock and just use the last example as that covers everything and avoids extra subprocesses (which is rather slow).

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Dec 19, 2022
* Avoid `should_not raise_error`
* Use mock() instead of manually doing it
@eregon eregon added this to the 23.0.0 Release milestone Dec 19, 2022
@jcouball
Copy link
Contributor Author

Thank you, Benoit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants