Make waiting_fd behaviour per-IO.
#13127
Open
+200
−376
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the “waiting_fd” behavior so that waiting and blocking operations are managed per‑IO rather than via a global list. Key changes include:
- Removing the global waiting_fd list initialization and associated memory‐size functions (e.g. in vm_core.h and vm.c).
- Replacing the waiting_fd structure and its helper functions in thread.c with per‑IO blocking operation nodes.
- Eliminating the thread_fd close test file, consistent with the new per‑IO design.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vm_core.h | Removed initialization of the global waiting_fds list. |
| vm.c | Removed calls to rb_vm_memsize_waiting_fds in memory size reporting. |
| thread.c | Commented out and removed waiting_fd structures/functions in favor of per‑IO nodes. |
| test/-ext-/thread_fd/test_thread_fd_close.rb | Removed outdated test file no longer applicable under the new design. |
| io.c | Updated blocking call APIs to accept an IO pointer instead of a raw file descriptor. |
| internal/* | Updated function declarations and struct fields to reflect per‑IO blocking operations. |
Files not reviewed (1)
- ext/-test-/thread_fd/depend: Language not supported
Comments suppressed due to low confidence (2)
vm.c:3229
- Verify that the new per‑IO blocking operations are correctly accounted for in the overall memory size calculations now that the global waiting_fd memory size contribution has been removed.
- rb_vm_memsize_waiting_fds(&vm->waiting_fds) +
test/-ext-/thread_fd/test_thread_fd_close.rb:1
- The removal of the thread_fd close test file reduces test coverage for FD closing behavior; please ensure that the per‑IO waiting/blocking operations introduced in this PR are adequately covered by other tests.
Entire file removed
9cba6ac to
5d85916
Compare
aa93db0 to
a0aa5d2
Compare
This comment has been minimized.
This comment has been minimized.
0ce6a12 to
bf8fd7d
Compare
bf8fd7d to
cbc4843
Compare
cbc4843 to
3d742b1
Compare
7976829 to
bcd3524
Compare
b8795d1 to
f18bd39
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Ruby VM’s waiting_fd behavior to operate on a per-IO basis. In doing so, the global waiting_fds list and its associated functions have been removed and replaced with per-IO blocking operations.
- Remove waiting_fds initialization and memory size calculations in vm_core.h and vm.c.
- Replace waiting_fd struct and functions in thread.c with a per-IO blocking_operations mechanism.
- Update test cases and IO-related functions (in io.c, internal/io.h, etc.) to reflect the new behavior.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vm_core.h, vm.c | Removed waiting_fds handling and its memory reporting |
| thread.c | Deleted waiting_fd struct and functions; introduced blocking_operations |
| test/ruby/test_io.rb | Adjusted file descriptor range to match expected fd limits |
| io.c, internal/io.h | Updated IO structure and related functions to support blocking_operations |
| internal/thread.h, gc.c | Revised internal interfaces, GC marking, and thread IO close functionality |
| ext/-test-/thread_fd/* | Removed redundant test extensions |
Files not reviewed (2)
- common.mk: Language not supported
- ext/-test-/thread_fd/depend: Language not supported
Comments suppressed due to low confidence (3)
test/ruby/test_io.rb:3829
- [nitpick] The adjustment of the range from (0..fd_setsize+1) to (0...fd_setsize) should be reviewed to ensure the test still covers the intended boundary conditions for file descriptor limits.
(0...fd_setsize).map {|i|
thread.c:1709
- With the removal of the waiting_fd mechanism and the introduction of rb_mutex_synchronize in io_blocking_operation_release, please verify that this change does not introduce any race conditions during unblocking.
rb_fiber_scheduler_unblock(thread->scheduler, io->self, rb_fiberptr_self(fiber));
io.c:5688
- [nitpick] The inclusion of the blocking_operations list in the memory size calculation is appropriate; please confirm that any future additions to the rb_io structure are also reflected in this computation to avoid memory under-reporting.
size += io->rbuf.capa;
f18bd39 to
a711798
Compare
a711798 to
76f56ff
Compare
a255ece to
498cf35
Compare
498cf35 to
402685f
Compare
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.
See #5384 for full context.