-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Remove shared sockets across epoll sets #283
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @bnbajwa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request authored by @bnbajwa.
The primary intent of this PR, as indicated by the title "feat: Remove shared sockets across epoll sets" and the associated task SDB-9184
, is to refactor the Reactor
class to prevent file descriptors (sockets) from being registered in multiple epoll sets simultaneously. This is achieved by introducing different internal structures for the Reactor
based on a new ReactorMode
.
The Reactor
now supports two modes: Blocking
and Immediate
. The Blocking
mode uses a single internal epoll instance, while the Immediate
mode uses two epoll instances (one for high priority and one for low priority). The choice of mode is made during Reactor
construction.
The core logic for managing file descriptors (subscribing, modifying events, unsubscribing, changing priority) has been updated to interact with the correct epoll instance(s) based on the ReactorMode
and the priority of the file descriptor, ensuring a file descriptor resides in only one epoll set at a time within a given Reactor
instance.
Example applications and unit tests have been updated to utilize the new ReactorMode
parameter in the constructor and, in the case of tests, to run the test cases against both modes using parameterized tests. The build configuration has also been updated to link the atomic
library, likely required for thread-safe operations introduced or utilized by the refactored Reactor
or its dependencies.
Highlights
- Reactor Refactoring: The
Reactor
class has been significantly refactored to support different operational modes usingstd::variant
to hold mode-specific internal state. - Introduction of ReactorMode: A new
ReactorMode
enum (Blocking
,Immediate
) is introduced to define the behavior of theReactor
's polling mechanism and internal epoll structure. - Mode-Specific Epoll Management: The internal epoll instances are now managed based on the
ReactorMode
.Blocking
mode uses one epoll,Immediate
mode uses two (high/low priority). File descriptors are registered in only one epoll set perReactor
instance. - Updated Reactor API: The
Reactor
constructor now requires aReactorMode
parameter. Internal methods likepoll
,subscribe
,set_events
,unsubscribe
, andset_io_priority
are updated to dispatch operations based on the active mode usingstd::visit
. - Parameterized Unit Tests: Existing
Reactor
unit tests are updated to run using parameterized test cases, ensuring coverage for bothBlocking
andImmediate
reactor modes. - Build Dependency: The
atomic
library is added as a link dependency for the core and benchmark libraries, suggesting the use of atomic operations within the refactored code.
Changelog
Click here to see the changelog
- example/EchoClnt.cpp
- Updated
Reactor
constructor call to specifyReactorMode::Blocking
(line 191).
- Updated
- example/EchoServ.cpp
- Updated
Reactor
constructor call to specifyReactorMode::Blocking
(line 143).
- Updated
- example/HttpServ.cpp
- Updated
Reactor
constructor call to specifyReactorMode::Blocking
(line 92).
- Updated
- example/Inotify.cpp
- Updated
Reactor
constructor call to specifyReactorMode::Blocking
(line 151).
- Updated
- toolbox/CMakeLists.txt
- Added
atomic
library to the link dependencies fortb-core-static
andtb-core-shared
(lines 135, 141). - Added
atomic
library to the link dependencies fortb-bm-static
andtb-bm-shared
(lines 155, 161). - Added
atomic
library to the link dependencies fortb-core-test
(line 267).
- Added
- toolbox/io/Reactor.cpp
- Included
<toolbox/util/Variant.hpp>
and<variant>
(lines 22, 24). - Changed
Reactor
constructor signature to acceptReactorMode
and initializedevice_
variant based on the mode (lines 48-59). - Updated
Reactor
destructor to usestd::visit
to delete the correct epoll instance(s) (lines 64-67). - Updated
subscribe
method to usestd::visit
to add the file descriptor to the correct epoll instance based on the mode (lines 78-81). - Renamed
poll
topoll_blocking
and addedBlockingDevice&
parameter (line 88). - Updated
poll_blocking
to use the epoll instance from theBlockingDevice
(lines 109, 112). - Added new
poll_immediate
method for handlingImmediateDevice
with separate high/low priority epoll polling (lines 141-187). - Refactored
poll
method to usestd::visit
to call eitherpoll_immediate
orpoll_blocking
(lines 189-194). - Updated
dispatch
method to use staticEpoll::fd
andEpoll::sid
(lines 275, 292). - Added
get_resident_epoll
helper function to retrieve the correct epoll instance from thedevice_
variant based on mode and priority (lines 316-324). - Updated
set_events
methods to useget_resident_epoll
and remove mode-specific epoll interaction logic (lines 332, 348, 360, 373). - Updated
unsubscribe
method to useget_resident_epoll
(line 383). - Updated
set_io_priority
method to usestd::visit
and handle priority changes within theImmediateDevice
structure (lines 395-406).
- Included
- toolbox/io/Reactor.hpp
- Included
<variant>
(line 25). - Defined
ReactorMode
enum (lines 34-42). - Changed
Reactor
constructor signature to acceptReactorMode
(line 144). - Defined
BlockingDevice
andImmediateDevice
structs (lines 197-204). - Moved
Data
struct definition (lines 206-211). - Replaced
epoll_
andhigh_prio_epoll_
members withstd::variant<BlockingDevice, ImmediateDevice> device_
(line 225). - Declared
get_resident_epoll
,poll_immediate
, andpoll_blocking
methods (lines 213-215).
- Included
- toolbox/io/Reactor.ut.cpp
- Included
<boost/test/data/test_case.hpp>
(line 24). - Added
param::ReactorModeWrapper
struct andreactor_modes
array for parameterized tests (lines 47-63). - Updated test cases (
ReactorLevelCase
,ReactorSocketPriority
,ReactorHighPriorityYield
,ReactorEdgeCase
,ReactorHookCase
,ReactorLowPriorityProgress
) to useBOOST_DATA_TEST_CASE
and acceptmode
parameter (lines 68, 100, 164, 375, 414, 428). - Updated
Reactor
constructor calls in test cases to use the parameterizedmode
(lines 72, 104, 168, 379, 419, 430). - Minor adjustments to polling logic and
CyclTime::now()
calls inReactorLowPriorityProgress
test (lines 461, 475).
- Included
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
An event loop, two modes now reside,
Blocking waits, while Immediate won't hide.
Sockets in one set,
No longer upset,
In their proper epoll, they now confide.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and well-implemented refactoring of the Reactor
class to support two distinct operational modes: Blocking
and Immediate
. The Immediate
mode now utilizes separate epoll instances for high and low priority file descriptors, effectively addressing the goal of "Remove shared sockets across epoll sets". This separation simplifies logic and enhances clarity for managing FDs in different priority groups.
The changes are comprehensive, touching the Reactor
's core logic, its public interface (constructor), CMake build files, example applications, and unit tests. The use of std::variant
for managing the two internal device states (BlockingDevice
, ImmediateDevice
) is a clean approach. Unit tests have been thoughtfully parameterized to cover both reactor modes, which is excellent for ensuring correctness.
Overall, the changes improve the flexibility and maintainability of the Reactor
. I have one medium-severity point regarding the handling of the notification file descriptor (notify_
) in Immediate
mode that might warrant discussion, but the rest of the implementation appears solid.
Summary of Findings
- Notification FD Handling in Immediate Mode: The notification FD (
notify_
) is added to the low-priority epoll set inImmediate
mode. This meansyield()
calls won't process wakeups. This might be an issue ifwakeup()
is intended as a high-priority interrupt. This has been raised as a medium severity comment. - Core Refactoring Quality: The refactoring to introduce
ReactorMode
and separate epoll management forImmediate
mode is well-executed usingstd::variant
. This directly addresses the PR's goal. - Test Coverage Enhancement: Unit tests have been effectively parameterized to cover both new reactor modes, which is a strong point of this PR.
- CMake
atomic
Linkage: The addition ofatomic
to linker dependencies in CMakeLists.txt is a safe practice ifstd::atomic
features are used that might require it on certain platforms. This is not an issue but a note on a related change. - Static Epoll Helper Methods: Changing
Epoll::fd()
andEpoll::sid()
to static methods is a good clarification, as they operate on the event data itself. (Not commented inline due to review settings, but noted as a positive change). - Unit Test Refinements: Moving
now = CyclTime::now();
beforer.poll()
calls in tests is a good refinement for accuracy. (Not commented inline due to review settings, but noted as a positive change).
Merge Readiness
The pull request is generally in good shape and implements a significant improvement to the Reactor's flexibility. The core logic changes are well-handled. There is one medium-severity point regarding the notify_
fd handling in Immediate
mode that should be discussed and potentially addressed. Once that point is clarified or resolved, the PR should be ready for merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally looks good, but my C++ is too dated to give full technical review.
SDB-9184