May 2023 - ruby-core - ml.ruby-lang.org

[ruby-core:113702] [Ruby master Misc#19122] Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack
by smcgivern (Sean McGivern) 30 May '23

30 May '23

Issue #19122 has been updated by smcgivern (Sean McGivern). ioquatix (Samuel Williams) wrote in #note-5: > If you want to use a specific mode (OS specific), you can do this: > > On Linux, find the mode, e.g. `MADV_DONTNEED = 6` (https://github.com/torvalds/linux/blob/933174ae28ba72ab8de5b35cb7c98fc21123…) > > Shift it one bit to the left (i.e. multiply by 2) `= 12`. > > Then run Ruby like this: > > ``` > > RUBY_SHARED_FIBER_POOL_FREE_STACKS=12 ruby > ``` Thanks for adding this! > As for making this the default, I suppose we could consider it but we'd need to confirm the performance impact. I would prefer to have high performance by default. RSS is a poor measurement IMHO. What about USS and PSS? Are they impacted the same way? Can we subtract the usage that can be later dropped? I mean, as I said in the original description, this is the only part of Ruby that works this way, which makes it surprising. If usage of fibers becomes more common in the wild, or `MADV_FREE` gets used in other areas where Ruby frees memory, then I would predict more reports noting this behaviour. Other languages have reverted their usage of `MADV_FREE` for similar reasons. We can construct another metric that gives us exactly what we want (for cAdvisor, we'd need something like https://github.com/google/cadvisor/issues/3197) but not through PSS or USS; both of those track RSS closely here, at least in my tests. We need to look at either `active_anon + inactive_anon` (for a cgroups `memory.stat` file), or `LazyFree` plus some other things I'm not sure about (for a process's `smaps_rollup`). This won't be the default in most monitoring tools, or in `top` et al, so as I said above, that wouldn't necessarily stop issues coming in. ---------------------------------------- Misc #19122: Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack https://bugs.ruby-lang.org/issues/19122#change-103348 * Author: smcgivern (Sean McGivern) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- I'd like to propose that Ruby stops using MADV_FREE when freeing a Fiber's stack, and switches to using MADV_DONTNEED even when MADV_FREE is supported. MADV_FREE is used in one place in the Ruby codebase, when freeing the stack of a freed Fiber: https://git.ruby-lang.org/ruby.git/tree/cont.c#n683 The comment for `fiber_pool_stack_free` says: ```c // We advise the operating system that the stack memory pages are no longer being used. // This introduce some performance overhead but allows system to relaim memory when there is pressure. ``` Where possible (i.e. on Linux 4.5 and later), `fiber_pool_stack_free` uses `MADV_FREE` over `MADV_DONTNEED`. This has the side effect that memory statistics such as RSS will not reduce until and unless the OS actually reclaims that memory. If that doesn't happen, then the reported memory usage via RSS will be much higher than the 'real' memory usage. If this was pervasive throughtout the Ruby codebase then that would be one thing, but currently this is just for Fiber. This means that: 1. A program that doesn't use Fiber will have somewhat reliable RSS statistics on recent Linux. 2. A program that heavily uses Fiber (such as something using Async::HTTP) will see an inflated RSS statistic. Go made a similar change to the one I'm proposing here for similar reasons: https://github.com/golang/go/issues/42330 > While `MADV_FREE` is somewhat faster than `MADV_DONTNEED`, it doesn't affect many of the statistics that `MADV_DONTNEED` does until the memory is actually reclaimed. This generally leads to poor user experience, like confusing stats in `top` and other monitoring tools; and bad integration with management systems that respond to memory usage. > [...] > I propose we change the default to prefer `MADV_DONTNEED` over `MADV_FREE`, to favor user-friendliness and minimal surprise over performance. I think it's become clear that Linux's implementation of `MADV_FREE` ultimately doesn't meet our needs. As an aside, MADV_FREE was not used in Ruby 3.1 (https://bugs.ruby-lang.org/issues/19101) and I haven't found any bugs filed about this behaviour other than that one. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113520] [Ruby master Bug#19680] test_process.rb tests fail sometimes on FreeBSD
by kjtsanaktsidis (KJ Tsanaktsidis) 29 May '23

29 May '23

Issue #19680 has been reported by kjtsanaktsidis (KJ Tsanaktsidis). ---------------------------------------- Bug #19680: test_process.rb tests fail sometimes on FreeBSD https://bugs.ruby-lang.org/issues/19680 * Author: kjtsanaktsidis (KJ Tsanaktsidis) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- I've been investigating the repeated failures of test_process.rb on FreeBSD on rubyci. I'm still working on it but I wanted to open this ticket just to keep others in the loop and gather any pointers any of you might have! These are some of the failures I've been investigating - * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230516T063002Z.f… * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230515T103002Z.f… * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230512T083002Z.f… * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230506T023001Z.f… * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230505T103002Z.f… * http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20230517T003001Z.f… -- https://bugs.ruby-lang.org/

2 5

[ruby-core:113698] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by ioquatix (Samuel Williams) 29 May '23

29 May '23

Issue #19057 has been updated by ioquatix (Samuel Williams). Status changed from Open to Closed https://github.com/ruby/ruby/pull/6511 was merged. In addition, the following related PRs were merged: - https://github.com/ruby/etc/pull/26 - https://github.com/ruby/io-wait/pull/25 - https://github.com/ruby/io-console/pull/43 ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103344 * Author: ioquatix (Samuel Williams) * Status: Closed * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113696] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by Eregon (Benoit Daloze) 29 May '23

29 May '23

Issue #19057 has been updated by Eregon (Benoit Daloze). ianks (Ian Ker-Seymer) wrote in #note-8: > > With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);. > > The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer). > > On the other, casting pointers to unsigned ints has implications for pointer provenance models. What do you mean? `struct RBasic*` to `VALUE` or vice-versa? Almost every C API function does that, so that seems a separate issue. Although maybe the above is not clear? `int rb_io_descriptor(VALUE io);` would be called with the Ruby (IO) object (like almost every other C API function), not with a pointer (opaque or not) to the `rb_io_t` struct. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103342 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113662] [Ruby master Feature#19695] mkmf: Adding the extra option --with-verbose to enable verbose mode.
by jaruga (Jun Aruga) 29 May '23

29 May '23

Issue #19695 has been reported by jaruga (Jun Aruga). ---------------------------------------- Feature #19695: mkmf: Adding the extra option --with-verbose to enable verbose mode. https://bugs.ruby-lang.org/issues/19695 * Author: jaruga (Jun Aruga) * Status: Open * Priority: Normal ---------------------------------------- I want the new feature to enable the verbose mode by setting the `V = 1` in the generated `Makefile` when building native extensions. Because the `V = 1` prints the compiler command lines. And it's useful to confirm and the compiler flags. It can be used like the commands below. ``` $ ruby /path/to/extconf.rb -- --with-verbose ``` ``` $ rake compiler -- --with-verbose ``` The pull-request is <https://github.com/ruby/ruby/pull/7863>. -- https://bugs.ruby-lang.org/

2 2

[ruby-core:113632] [Ruby master Bug#19691] Case insensitive file systems, require filename casing
by MSP-Greg (Greg L) 28 May '23

28 May '23

Issue #19691 has been reported by MSP-Greg (Greg L). ---------------------------------------- Bug #19691: Case insensitive file systems, require filename casing https://bugs.ruby-lang.org/issues/19691 * Author: MSP-Greg (Greg L) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- I don't know what I think of this (expected or bug), but recently in a forum I saw an issue that I don't recall accidentally doing myself. An equivalent example: ``` ruby -rsingleton -rSingleton -e "puts 'case issue'" ``` Note the change of casing with `Singleton`. On case insensitive file systems, the above will double load the file, which errors. Obviously, on case sensitive file systems, it errors with `cannot load such file -- Singleton` So, options are: A. No change to current behavior, which I'm fine with. B. If the OS is case insensitive, don't reload the file, and maybe a warning that `Singleton` and `singleton` are equivalent. -- https://bugs.ruby-lang.org/

3 6

[ruby-core:113689] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by ianks (Ian Ker-Seymer) 27 May '23

27 May '23

Issue #19057 has been updated by ianks (Ian Ker-Seymer). > With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);. > The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer). I have mixed feelings about this. On one hand, it does successfully hide the implementation. On the other, casting pointers to unsigned ints has implications for pointer provenance models. Would be nice to get the opaqueness without the int-cast. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103334 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113682] [Ruby master Misc#17591] Test frameworks and REPLs do not show deprecation warnings by default
by Eregon (Benoit Daloze) 27 May '23

27 May '23

Issue #17591 has been updated by Eregon (Benoit Daloze). IMO we should find a way for RSpec to enable them by default, it is hurting RSpec users and destroying ruby-core's efforts to provide nice migration paths. Maybe more voices on that RSpec issue could help to convince the RSpec devs (but probably not). Or potentially workaround by enabling them e.g. when running tests of a rails app. RSpec does have `-w` which enables them, so another potential way could be to teach rspec users about that, maybe via a gem-install-time message? RSpec users will likely learn about it when something breaks without warning, but then they will probably think it's CRuby's fault when it's RSpec's fault. I agree enabling deprecation warnings by default seems best, and then people who don't want to see it can just set `Warning[:deprecated] = false`. Let's see what others think, but I suspect many would be reticent to change this again. ---------------------------------------- Misc #17591: Test frameworks and REPLs do not show deprecation warnings by default https://bugs.ruby-lang.org/issues/17591#change-103326 * Author: Eregon (Benoit Daloze) * Status: Closed * Priority: Normal ---------------------------------------- Various people in #16345 said that: > The issue can be mitigated if all test frameworks enable all deprecation warnings. > The developer's practice can be supported by tools, such as test frameworks enable deprecation warnings automatically. And this was used as a base to disable deprecation warnings by default in Ruby 2.7.2. However, it seems no test frameworks or REPLs actually show deprecation warnings by default! So Ruby developers will typically never see deprecation warnings for keyword arguments, and it will just break when they try Ruby 3.0. I think only MSpec does `Warning[:deprecated] = true`, whether or not `-w` is passed, which seems the right thing to do. Currently, RSpec enables `Warning[:deprecated] = true` only for `rspec -w`. Same for `test/unit` 3.3.4 shipped with 2.7.2. IRB in 2.7.2 does not show deprecated warnings. Same for `pry`. I think ruby-core needs to have a clear message here, like: > All test frameworks and REPLs should include this snippet and run it before running tests: `Warning[:deprecated] = true if Warning.respond_to?(:[]=)`. > This is important so that developers see warnings in development, and that they see the warnings before updating to the next Ruby version. > Developers can choose to disable deprecation warnings explicitly if they want with `Warning[:deprecated] = false`. And I think it would be good that ruby-core makes a PR or an issue to the main test frameworks/REPLs to show examples. P.S.: if someone wants to disable all warnings with `-W0` or `$VERBOSE = nil`, it will indeed disable them all, including deprecation warnings, so there is no need to check `$VERBOSE` for setting `Warning[:deprecated] = true`. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113681] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by ioquatix (Samuel Williams) 27 May '23

27 May '23

Issue #19057 has been updated by ioquatix (Samuel Williams). Given the discussion here, I just want to aim for "Hide all the details". Thanks everyone, the discussion has been most helpful. I especially like the discussions around undefined behaviour. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103325 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113680] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by Eregon (Benoit Daloze) 27 May '23

27 May '23

Issue #19057 has been updated by Eregon (Benoit Daloze). ioquatix (Samuel Williams) wrote in #note-3: > Maybe it's CRuby problem, but there is a performance issue with such an implementation which has to extract and follow the pointer to the struct over and over again. It's pretty rare to access multiple fields of `rb_io_t` in code I have seen, so then performance-wise it is the same if accessing a single field. When accessing two fields it would be indeed an extra indirection, although I highly doubt this would be measurable overhead. Coming from the other way, on all Ruby implementations besides CRuby, exposing `struct rb_io *ioptr` would mean allocating a struct just to support those accesses, for every IO instance, possibly lazily. That's a much much bigger overhead than an extra pointer indirection. As HPy shows, a fast and portable C API does not expose structs. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103324 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0