Skip to content

Hide most of the implementation of struct rb_io. #6511

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 30, 2023
Merged

Hide most of the implementation of struct rb_io. #6511

merged 18 commits into from
May 30, 2023

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Oct 8, 2022 •
edited
Loading

@ioquatix ioquatix force-pushed the io-hide-implementation branch 2 times, most recently from 4cfeb71 to ab41c3b Compare October 8, 2022 06:07
@ioquatix ioquatix requested review from nobu, akr and ko1 October 10, 2022 21:39
eregon
eregon reviewed Oct 13, 2022
View reviewed changes
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.

Looks good to me.

@ioquatix
Copy link
Member Author

@eregon do you know if this is in violation of any C specification or potentially creates undefined behaviour for us?

@ioquatix ioquatix force-pushed the io-hide-implementation branch from fcdfafa to 55a028c Compare October 14, 2022 11:01
@eregon
Copy link
Member

eregon commented Oct 14, 2022

@eregon do you know if this is in violation of any C specification or potentially creates undefined behaviour for us?

I don't know.

@ioquatix ioquatix force-pushed the io-hide-implementation branch from 1c0cc42 to 563ea1d Compare October 14, 2022 11:12
@ioquatix ioquatix requested a review from eregon October 14, 2022 11:16
@tenderlove
Copy link
Member

This looks good and makes sense to me!

ianks
ianks reviewed Oct 14, 2022
View reviewed changes
internal/io.h
int len;

/** Designed capacity of the buffer. */
int capa;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as why unsigned int here

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it's odd. But not part of my design, it's what's copied.

@ioquatix ioquatix mentioned this pull request May 26, 2023
Merged
@ioquatix ioquatix force-pushed the io-hide-implementation branch from 563ea1d to e3b529d Compare May 27, 2023 10:25
This was referenced May 28, 2023
Merged
Merged
@ioquatix ioquatix force-pushed the io-hide-implementation branch from cfcec68 to 8823f74 Compare May 28, 2023 05:22
@ioquatix ioquatix mentioned this pull request May 28, 2023
Merged
@KJTsanaktsidis KJTsanaktsidis mentioned this pull request May 29, 2023
Closed
@ioquatix ioquatix force-pushed the io-hide-implementation branch 2 times, most recently from 882a084 to cbe5d12 Compare May 29, 2023 05:23
@ioquatix ioquatix mentioned this pull request May 29, 2023
Merged
@ioquatix ioquatix force-pushed the io-hide-implementation branch 5 times, most recently from a4551b3 to ee613f8 Compare May 29, 2023 15:14
ioquatix and others added 16 commits May 30, 2023 07:06
@ioquatix
Fix incorrect usage.
298ded0
@ioquatix
Add comments.
7fc2120
@ioquatix
Hide public struct.
313c0cb
@ioquatix
Add rb_io_path and rb_io_open_descriptor.
3bf26d5
@ioquatix
Expose IO internals to objspace.
5a0fb8d
@ioquatix
Use rb_io_open_descriptor to create PTY objects
8bd33e2
@ioquatix
Keep naming the slave PTY as simply slavename
40db0e4 
I had thought that "slavepty:$slavename" was a better name, but there
are tests for the current name so somebody is probably depending on the
current behaviour.
@ioquatix
Rename FMODE_PREP -> FMODE_EXTERNAL and expose it
52c4077 
FMODE_PREP I believe refers to the concept of a "pre-prepared" file, but
FMODE_EXTERNAL is clearer about what the file descriptor represents and
aligns with language in the IO::Buffer module.
@ioquatix
Ensure that rb_io_open_descriptor closes the FD if it fails
ed59af3 
If FMODE_EXTERNAL is not set, then it's guaranteed that Ruby will be
responsible for closing your file, eventually, if you pass it to
rb_io_open_descriptor, even if it raises an exception.
@ioquatix
Rename IS_EXTERNAL_FD -> RUBY_IO_EXTERNAL_P
20e6567
@ioquatix
Expose rb_io_closed_p.
c8e72b3
@ioquatix
Update dependencies
624dbb8
@ioquatix
Restore compatibility #to_path returns dup.
a425905
@ioquatix
win32.c compatibility.
bae053e
@ioquatix
Prefer rb_io_descriptor in io_spec.c.
18e5b6e
@ioquatix
Add rb_io_mode to get IO mode.
74b830b
@ioquatix ioquatix force-pushed the io-hide-implementation branch from ee613f8 to 74b830b Compare May 29, 2023 22:06
@ioquatix
Copy link
Member Author

@KJTsanaktsidis if you have time, do you mind reviewing the last set of commits?

@ioquatix ioquatix merged commit 18e55fc into ruby:master May 30, 2023
@ioquatix ioquatix deleted the io-hide-implementation branch May 30, 2023 01:02
KJTsanaktsidis
KJTsanaktsidis reviewed May 30, 2023
View reviewed changes
Copy link
Contributor

@KJTsanaktsidis KJTsanaktsidis left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple of teeny tiny issues

debug.c
@@ -70,7 +70,7 @@ const union {
RUBY_FMODE_NOREVLOOKUP = 0x00000100,
RUBY_FMODE_TRUNC = FMODE_TRUNC,
RUBY_FMODE_TEXTMODE = FMODE_TEXTMODE,
RUBY_FMODE_PREP = 0x00010000,
RUBY_FMODE_EXTERNAL = 0x00010000,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote this, but since FMODE_EXTERNAL is exported now, this should readRUBY_FMODE_EXTERNAL = FMODE_EXTERNAL rather than duplicating its definition.

io.c
VALUE io = io_alloc(klass);
VALUE path_value = Qnil;
if (path) {
path_value = rb_obj_freeze(rb_str_new_cstr(path));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suuuuper nitpicky, but - if rb_str_new_cstr raises, this will then leak the file descriptor (because rb_io_open_descriptor won't have been called).

I also just realised the usages of rb_io_open_descriptor I added in pty.c have the same problem - they open a file, then construct a path (in a way that can raise), and then call rb_io_open_descriptor with said path.

I wonder if the API might be safer if we stop rb_io_open_descriptor from taking a path at all, and instead expose a rb_io_set_path? That way the path can be constructed after the FD is safely put under the management of the GC.

(or, we might decide we just don't care - probably the only way these string allocations can raise is out-of-memory conditions or very silly tracepoints?)

@ioquatix ioquatix mentioned this pull request May 30, 2023
Closed
@eregon eregon mentioned this pull request May 30, 2023
Merged
matzbot pushed a commit that referenced this pull request May 31, 2023
@nurse
Revert "Hide most of the implementation of struct rb_io. (#6511)"
85dcc48 
This reverts commit 18e55fc.

fix [Bug #19704]
https://bugs.ruby-lang.org/issues/19704
This breaks compatibility for extension libraries. Such changes
need a discussion.
@ioquatix ioquatix mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KJTsanaktsidis KJTsanaktsidis KJTsanaktsidis left review comments

@ianks ianks ianks left review comments

@nobu nobu Awaiting requested review from nobu

@akr akr Awaiting requested review from akr

@ko1 ko1 Awaiting requested review from ko1

@eregon eregon Awaiting requested review from eregon

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ioquatix @eregon @tenderlove @KJTsanaktsidis @ianks