fix: Add classes to clean-up Spider's internal Data, Driver, and Job objects from the database when clients destroy the corresponding client objects (fixes #135). - #136
Conversation
WalkthroughThis change set introduces new "Cleaner" classes— Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Driver/TaskContext
participant Core as core::{Context, DataCleaner, DriverCleaner, JobCleaner}
participant Storage as MetadataStorage/DataStorage
participant Factory as StorageFactory
participant Conn as StorageConnection
Note over Client,Core: Resource Creation (Data, Driver, Job)
Client->>Core: Create Context (Source, UUID)
Client->>Core: Create Cleaner (DataCleaner/DriverCleaner/JobCleaner) with Context, Storage, Factory, Conn
Note over Core: On object destruction
Core->>Core: Check for exception unwinding
alt Not unwinding
Core->>Conn: Use existing or acquire from Factory
alt DataCleaner
Core->>Storage: Remove driver/task reference to data
else DriverCleaner
Core->>Storage: Remove driver metadata
else JobCleaner
Core->>Storage: Remove job metadata
end
else Unwinding
Core-->>Core: Skip cleanup
end
sequenceDiagram
participant Client as Client
participant Core as core::Context
participant Storage as MetadataStorage
Client->>Core: Create Context (Source, UUID)
Client->>Storage: Remove driver/job/scheduler using Context info
Possibly related PRs
Possibly related issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
src/spider/client/Data.hpp (1)
116-147:⚠️ Potential issueUser-supplied cleanup function is collected but never wired – cleanup silently lost
Builder::set_cleanup_funcstoresm_cleanup_func, yetbuild()never transfers it to the newly createdcore::DataorDataCleaner. As a result, the caller’s cleanup logic is never executed, defeating the purpose of the API.msgpack::sbuffer buffer; msgpack::pack(buffer, t); auto data = std::make_unique<core::Data>(std::string{buffer.data(), buffer.size()}); data->set_locality(m_nodes); data->set_hard_locality(m_hard_locality); +if (m_cleanup_func) { + data->set_cleanup_func(m_cleanup_func); // <-- preserve user intent +}If
core::Datano longer supports arbitrary cleanup, deprecateset_cleanup_funccompletely to avoid a misleading interface.Either wire the function as above or remove the builder method; otherwise clients will assume their resources are freed while they are not.
🧹 Nitpick comments (8)
src/spider/scheduler/scheduler.cpp (1)
276-278: Consider adding cleanup to error paths.While the current implementation handles cleanup on normal termination, consider also adding similar cleanup calls in error paths (e.g., in the catch blocks or before early returns due to errors) to ensure consistent cleanup regardless of how the program exits.
src/spider/core/DriverCleaner.cpp (1)
15-45: Consider deduplicating the three almost-identical Cleaner classes
DataCleaner,JobCleaner, andDriverCleanershare ~90 % of their code. A small CRTP/base-class abstraction (e.g.,BaseCleaner) parameterised with the removal lambda would eliminate duplication, reduce maintenance overhead, and ensure bug-fixes (like the two above) propagate automatically.src/spider/core/DriverCleaner.hpp (1)
48-50: Minor: comment in include-guard footer is inconsistentThe opening guard is
SPIDER_CORE_DRIVERCLEANER_HPP, but the footer says// DRIVERCLEANER_HPP.
Aligning them improves readability:-#endif // DRIVERCLEANER_HPP +#endif // SPIDER_CORE_DRIVERCLEANER_HPPsrc/spider/core/JobCleaner.hpp (1)
48-50: Synchronise include-guard footer comment-#endif +#endif // SPIDER_CORE_JOB_CLEANER_HPPsrc/spider/client/Job.hpp (2)
263-288: Consider extracting repeated context–based data-fetch logic into a helperThe conditional branches that decide between
get_driver_datavsget_task_data(lines 263-288 and again at 339-364) are identical. Duplicating this block:if (m_context.get_source() == core::Context::Source::Driver) { err = m_data_storage->get_driver_data(conn, m_context.get_id(), data_id, &data); } else { err = m_data_storage->get_task_data(conn, m_context.get_id(), data_id, &data); }means any future change (e.g., adding a new
Sourceenum) must be done in two places and increases cognitive load.A tiny, private helper keeps the intent clear and shrinks the hot path:
+auto fetch_data(core::StorageConnection& conn, + boost::uuids::uuid const& data_id, + core::Data* out) -> core::StorageErr { + if (m_context.get_source() == core::Context::Source::Driver) { + return m_data_storage->get_driver_data(conn, m_context.get_id(), data_id, out); + } + return m_data_storage->get_task_data(conn, m_context.get_id(), data_id, out); +} ... - if (m_context.get_source() == core::Context::Source::Driver) { - err = m_data_storage->get_driver_data(conn, m_context.get_id(), data_id, &data); - } else { - err = m_data_storage->get_task_data(conn, m_context.get_id(), data_id, &data); - } + err = fetch_data(conn, data_id, &data);This will keep the method length under control and prevent subtle divergences later.
Also applies to: 339-364
388-391:m_job_cleanerhas sole ownership – ensure its lifetime is necessary
Jobnow stores astd::unique_ptr<core::JobCleaner>. Because the pointer is never accessed insideJob, its only purpose is RAII-side-effects in the destructor. That is correct, but:
- Document this with a short comment so future readers don’t assume it is dead code.
- Consider
[[maybe_unused]] std::unique_ptr…or naming itm_cleanup_guardto make the intent self-evident.Tiny clarity tweak, no functional change.
src/spider/client/Data.hpp (2)
132-146: Add adefaultbranch to future-proof theswitchonContext::SourceCurrently only
DriverandTaskare handled. Adding adefaultthat throws makes the code robust against enum extension and silences some compiler warnings with-Wswitch-enum.switch (m_context.get_source()) { case core::Context::Source::Driver: … case core::Context::Source::Task: … + default: + throw ConnectionException{"Unhandled Context::Source value"}; }Minor defensive hardening.
174-176: Rename and/or annotate the hidden guard members for clarity
m_context&m_data_cleanerexist only for side-effects and are not referenced elsewhere. A brief comment or a more explicit name (e.g.,m_context_guard,m_data_cleanup_guard) will prevent future “unused-member?” confusion.Also applies to: 217-218
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/spider/CMakeLists.txt(2 hunks)src/spider/client/Data.hpp(4 hunks)src/spider/client/Driver.cpp(2 hunks)src/spider/client/Driver.hpp(5 hunks)src/spider/client/Job.hpp(7 hunks)src/spider/client/TaskContext.hpp(4 hunks)src/spider/core/Context.hpp(1 hunks)src/spider/core/DataCleaner.cpp(1 hunks)src/spider/core/DataCleaner.hpp(1 hunks)src/spider/core/DataImpl.hpp(2 hunks)src/spider/core/DriverCleaner.cpp(1 hunks)src/spider/core/DriverCleaner.hpp(1 hunks)src/spider/core/JobCleaner.cpp(1 hunks)src/spider/core/JobCleaner.hpp(1 hunks)src/spider/scheduler/scheduler.cpp(1 hunks)src/spider/storage/MetadataStorage.hpp(1 hunks)src/spider/storage/mysql/MySqlStorage.cpp(1 hunks)src/spider/storage/mysql/MySqlStorage.hpp(1 hunks)src/spider/worker/FunctionManager.hpp(1 hunks)tests/storage/test-MetadataStorage.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/spider/scheduler/scheduler.cpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(41-41)conn(44-47)conn(49-55)conn(57-58)conn(60-61)conn(63-64)conn(66-70)conn(73-74)conn(76-80)conn(82-82)conn(83-83)conn(84-85)
tests/storage/test-MetadataStorage.cpp (2)
tests/integration/client.py (1)
storage(79-82)src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(41-41)conn(44-47)conn(49-55)conn(57-58)conn(60-61)conn(63-64)conn(66-70)conn(73-74)conn(76-80)conn(82-82)conn(83-83)conn(84-85)
src/spider/storage/mysql/MySqlStorage.hpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(41-41)conn(44-47)conn(49-55)conn(57-58)conn(60-61)conn(63-64)conn(66-70)conn(73-74)conn(76-80)conn(82-82)conn(83-83)conn(84-85)
src/spider/client/Driver.hpp (2)
src/spider/client/Driver.cpp (2)
Driver(24-72)Driver(74-120)src/spider/core/Driver.hpp (2)
Driver(12-12)Driver(12-12)
src/spider/core/JobCleaner.hpp (1)
src/spider/core/JobCleaner.cpp (2)
JobCleaner(16-25)JobCleaner(27-45)
src/spider/core/DataCleaner.hpp (1)
src/spider/core/DataCleaner.cpp (2)
DataCleaner(17-28)DataCleaner(30-52)
src/spider/core/DriverCleaner.hpp (1)
src/spider/core/DriverCleaner.cpp (2)
DriverCleaner(16-25)DriverCleaner(27-45)
🪛 Cppcheck (2.10-2)
src/spider/client/Driver.cpp
[performance] 101-101: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (26)
src/spider/scheduler/scheduler.cpp (1)
276-278: Good addition for proper driver cleanup.This cleanup call ensures that the driver data associated with the scheduler is properly removed from storage when the scheduler terminates normally, preventing resource leaks for long-running clients.
src/spider/worker/FunctionManager.hpp (1)
317-317: Good use of Context for proper resource tracking.The addition of the Context parameter with the Task source type ensures that data objects created during function invocation are properly associated with their origin task. This enables the new cleaner classes to perform appropriate cleanup when these objects are destructed.
src/spider/storage/mysql/MySqlStorage.hpp (1)
43-43: Good implementation of the new interface method.The addition of the
remove_drivermethod declaration in the MySQL storage implementation aligns with the interface changes and enables proper cleanup of driver metadata from the database.src/spider/storage/MetadataStorage.hpp (1)
35-41: Well-documented interface addition for driver cleanup.The new
remove_drivermethod with clear documentation is a good addition to the storage interface. This method enables the cleaner classes to properly remove driver metadata from storage when objects are destructed, addressing the issue mentioned in the PR objectives.tests/storage/test-MetadataStorage.cpp (3)
72-74: Good addition of cleanup step.Adding explicit driver removal after the test is completed ensures resources are properly managed and prevents potential side effects on other tests.
107-108: Proper resource cleanup after test completion.Adding explicit driver removal for the scheduler instance ensures proper cleanup after testing scheduler address functionality.
602-602: Consistent resource cleanup implementation.This adds the final missing driver cleanup step for the scheduler lease timeout test, ensuring consistent resource management across all test cases.
src/spider/client/Driver.cpp (2)
47-52: Well-placed resource cleanup initialization.The
DriverCleaneris constructed after successfully adding the driver to storage but before starting the heartbeat thread, which is the appropriate sequence to ensure proper lifecycle management.
95-100: Good implementation of consistent cleanup across constructors.Both constructors now initialize the driver cleaner in the same way, ensuring consistent resource management regardless of how the Driver object is created.
src/spider/core/DataImpl.hpp (3)
9-9: Appropriate addition of Context header.Including the Context header enables the new context-aware creation functionality.
18-18: Good integration of Context parameter.Adding the Context parameter to create_data() aligns with the broader architectural shift towards unified source identification across the codebase.
22-22: Correct usage of context in Data constructor.The context parameter is properly passed to the Data constructor, maintaining the ownership semantics with the other moved and shared parameters.
src/spider/CMakeLists.txt (2)
3-5: Complete addition of new cleaner source files.All three cleaner source files (DataCleaner, DriverCleaner, JobCleaner) are properly included in the build system, ensuring the implementation is available to the core library.
19-25: Comprehensive header file inclusion.All required headers for the new context and cleaner functionality are properly added to the core headers list, making them accessible to dependent components.
src/spider/client/Driver.hpp (5)
17-17: Good addition of the DriverCleaner include.This include is necessary for the newly added member variable that manages driver cleanup.
87-87: Appropriate use of the new Context abstraction.The refactoring to use core::Context for source identification is a good pattern that unifies how source information is represented throughout the codebase.
231-231: Consistent use of Context for Job initialization.The change maintains consistency with the other places where Context is used for source identification.
295-295: Consistent Context usage in TaskGraph start method.Good to see the consistent application of the Context pattern across all methods that create Jobs.
323-323: Good implementation of resource cleanup with DriverCleaner.This addresses the issue described in PR #135 where driver objects weren't being removed from storage. The unique_ptr ensures the cleaner's destructor is called when the Driver is destroyed, properly cleaning up driver metadata.
src/spider/storage/mysql/MySqlStorage.cpp (1)
221-241: Well-implemented driver removal method.The implementation follows the established patterns in the codebase with proper transaction handling and error classification. This method appropriately enables the removal of driver entries from the database, which is essential for the cleanup solution described in the PR.
Points of strength:
- Follows consistent error handling pattern with rollback on failure
- Correctly differentiates KeyNotFoundErr from generic errors
- Properly commits transaction on success
src/spider/client/TaskContext.hpp (4)
18-18: Appropriate addition of Context include.This include is necessary for using the new Context class that unifies source identification.
65-65: Good use of Context for DataBuilder initialization.Consistent with the pattern established in Driver.hpp, using Context to encapsulate the source type and ID.
177-177: Proper Context usage for Job creation.This change maintains consistency with other Job creation code in the system.
235-235: Consistent Context usage in TaskGraph start method.Good to see the Context pattern applied consistently across all methods that create Jobs.
src/spider/core/Context.hpp (1)
1-33: Well-designed Context abstraction.This new class is a clean and focused abstraction for execution context information. It encapsulates both the source type (Driver or Task) and a UUID identifier, which simplifies interfaces throughout the system.
Strengths:
- Uses enum class for type safety
- Good encapsulation with private members and accessor methods
- Appropriate use of [[nodiscard]] attribute
- Simple and focused design that serves its purpose well
This abstraction helps unify how source context is represented across the codebase, making interfaces more consistent and supporting the cleaner classes introduced in this PR.
src/spider/core/DataCleaner.cpp (1)
30-35:Details
✅ Verification successful
Verify
m_num_exceptionsis initialised at construction timeThe comparison against
m_num_exceptionsassumes this member captured the value returned bystd::uncaught_exceptions()in the constructor. Ifm_num_exceptionsis default-initialised to 0, the destructor will always believe it is running during stack-unwinding, and the cleanup will never execute.
Please confirm it is initialised and, if not, add the initialisation.
🏁 Script executed:
#!/bin/bash # Ensure m_num_exceptions is assigned in the header / constructor rg -n "m_num_exceptions" -g "*DataCleaner*.[ch]pp"Length of output: 220
No action needed:
m_num_exceptionsis already initialised
The default member initialiser inDataCleaner.hppsetsint m_num_exceptions = std::uncaught_exceptions();so the destructor’s comparison is valid and no further initialisation is required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/spider/core/JobCleaner.cpp (2)
33-42:⚠️ Potential issueMissing null check for storage factory
The code doesn't check if
m_storage_factoryis null before attempting to use it, which could cause a crash.Add a null check before attempting to use the storage factory:
if (nullptr == conn) { + if (!m_storage_factory) { + return; // cannot obtain connection + } std::variant<std::unique_ptr<StorageConnection>, StorageErr> conn_result = m_storage_factory->provide_storage_connection();
43-44:⚠️ Potential issueAdd exception handling for
remove_jobThe call to
remove_jobmay throw exceptions, which would be problematic in a destructor. If an exception escapes a destructor during stack unwinding,std::terminatewill be called.Protect the call with a try/catch block:
- m_metadata_store->remove_job(*conn, m_job_id); + try { + m_metadata_store->remove_job(*conn, m_job_id); + } catch (...) { + // Log error or handle silently to prevent std::terminate + }src/spider/core/DataCleaner.hpp (1)
32-32: 🛠️ Refactor suggestionDestructor should be marked
noexceptThe destructor can call methods that interact with storage, potentially leading to exceptions. If an exception escapes the destructor during stack unwinding,
std::terminatewill be called.- ~DataCleaner(); + ~DataCleaner() noexcept;src/spider/core/DriverCleaner.hpp (1)
31-31: 🛠️ Refactor suggestionDestructor should be marked
noexceptThe destructor can call methods that interact with storage and performs I/O operations (
std::coutin the implementation), which could potentially lead to exceptions. If an exception escapes the destructor during stack unwinding,std::terminatewill be called.- ~DriverCleaner(); + ~DriverCleaner() noexcept;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/spider/core/DataCleaner.hpp(1 hunks)src/spider/core/DriverCleaner.cpp(1 hunks)src/spider/core/DriverCleaner.hpp(1 hunks)src/spider/core/JobCleaner.cpp(1 hunks)src/spider/core/JobCleaner.hpp(1 hunks)src/spider/storage/DataStorage.hpp(2 hunks)src/spider/storage/MetadataStorage.hpp(2 hunks)src/spider/storage/mysql/MySqlStorage.cpp(4 hunks)src/spider/storage/mysql/MySqlStorage.hpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/spider/storage/MetadataStorage.hpp
- src/spider/core/DriverCleaner.cpp
- src/spider/core/JobCleaner.hpp
- src/spider/storage/mysql/MySqlStorage.hpp
- src/spider/storage/mysql/MySqlStorage.cpp
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/spider/core/DataCleaner.hpp (1)
src/spider/core/DataCleaner.cpp (2)
DataCleaner(17-28)DataCleaner(30-52)
src/spider/core/DriverCleaner.hpp (1)
src/spider/core/DriverCleaner.cpp (2)
DriverCleaner(16-25)DriverCleaner(27-46)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (12)
src/spider/storage/DataStorage.hpp (2)
73-77: Good addition ofnoexceptspecifier forremove_task_referenceAdding the
noexceptspecifier to this method is appropriate since it will be called during cleanup operations in destructors. This ensures the method won't throw exceptions during resource cleanup, preventing potentialstd::terminatecalls if exceptions occur during stack unwinding.
85-89: Good addition ofnoexceptspecifier forremove_driver_referenceAdding the
noexceptspecifier to theremove_driver_referencemethod is appropriate for the same reason as withremove_task_reference. This will ensure safe cleanup operations during destruction of objects.src/spider/core/JobCleaner.cpp (2)
16-25: Constructor looks good with appropriate resource managementThe constructor properly initializes member variables using move semantics for the shared pointers, which is efficient and maintains correct ownership semantics.
27-32: Good stack unwinding detection in destructorThe destructor correctly checks if it's being called during stack unwinding by comparing the current exception count with the count at construction time. This prevents cleanup operations during exception propagation, which is a good practice.
src/spider/core/DataCleaner.hpp (4)
14-22: Good documentation explaining the class purpose and designThe class documentation clearly explains the reason for this cleaner class and why it can't be directly integrated into the
Dataclass. The explanation about moved objects is particularly useful.
25-31: Constructor appropriately captures all required referencesThe constructor parameters include all necessary components for safely managing data cleanup: the data ID, context, and various storage-related shared pointers.
34-39: Good ownership semantics with deleted copy operationsCorrectly deleting copy operations while allowing move operations prevents unintended duplication of cleaners that could lead to double-deletion of resources.
41-48: Member variables appropriately capture required stateThe member variables properly capture all the state needed for cleanup operations, including the exception count at construction. The default nullptr for the connection is appropriate as it will be obtained only when needed.
src/spider/core/DriverCleaner.hpp (4)
14-21: Good documentation explaining the class purpose and designThe class documentation clearly explains the rationale behind this cleaner class and why it can't be directly integrated into the
Driverclass, particularly regarding moved objects.
24-29: Constructor appropriately captures all required referencesThe constructor parameters include all necessary components for safely managing driver cleanup: the driver ID and various storage-related shared pointers.
33-38: Good ownership semantics with deleted copy operationsCorrectly deleting copy operations while allowing move operations prevents unintended duplication of cleaners that could lead to double-deletion of driver resources.
40-46: Member variables appropriately capture required stateThe member variables properly capture all the state needed for cleanup operations, including the exception count at construction. The default nullptr for the connection is appropriate as it will be obtained only when needed.
Co-authored-by: davidlion <davidlion2@protonmail.com>
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
- We should mention why exceptions pose an issue in the Cleaner doc strings (and the PR description).
- Update the title.
- Update the description.
sitaowang1998
changed the title
|
@coderabbitai, based on the code and PR description, what's a good title for this pull request? |
|
Tip For best results, initiate chat on the files or code changes. @kirkrodrigues Based on the code changes and PR description, here are some improved title suggestions that better capture the scope and purpose of this PR: Top recommendations:
Alternative options:
The current title is functional, but these suggestions better emphasize:
I'd recommend option 1 as it clearly states both the problem (resource leaks in long-running sessions) and the solution approach (RAII cleanup), which captures the essence of this substantial architectural improvement. |
sitaowang1998
changed the title
3 participants
Description
Problem
In the current implementation, Spider clients do not clean up
Data,Job, orDriverobjects from storage when the corresponding client-side objects are destructed. As a result, in long-running clients, stale data and jobs persist in storage because their associated drivers still hold valid references, preventing automatic cleanup. This issue is tracked in #135.Solution
This PR addresses the problem by introducing specialized cleaner classes:
DataCleaner,JobCleaner, andDriverCleaner. These classes ensure that associatedData,Job, andDriverentries are properly removed from storage when the client-side objects are destructed.Exception Safety
To avoid unintended cleanup during exception handling, the destructors of the cleaner classes use
std::uncaught_exceptionsto determine if the program is currently unwinding the stack due to an exception. If so, the cleanup is skipped.This ensures that cleanup is performed only during normal object destruction, and not during exception handling, preserving exception safety and system stability.
Move Safety with
std::unique_ptrTo prevent cleanup from being executed on moved-from objects (which could lead to double-deletion), the cleaner objects are owned via
std::unique_ptr. This ensures:unique_ptrbecomesnullptr, so the cleaner's desctructor is not called.By relying on
std::unique_ptr, we ensure that the cleaner's destructor is only called once per resource, and only by the last owner. This avoids accidental cleanup of moved objects.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores