fix: Use std::nothrow to disable exceptions throwing in new operators (fixes #103).
#105
Conversation
WalkthroughThe pull request introduces modifications across several files, primarily enhancing memory management and error handling by utilizing Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (3)
src/clp_ffi_py/ir/native/PyDeserializer.cpp (1)
292-297: Consider tracking the TODO comment for future improvement.While the implementation correctly uses
std::nothrow, the TODO comment indicates that a more appropriate error code should be used when user-defined error codes are supported.Would you like me to create a GitHub issue to track this TODO item for implementing proper error codes?
src/clp_ffi_py/ir/native/PyQuery.cpp (1)
Line range hint
632-644: LGTM! Proper handling of memory allocation failure.The implementation correctly uses
new (std::nothrow)and checks for allocation failure, setting an appropriate Python runtime error when allocation fails. This change aligns with the PR objectives to improve memory allocation failure handling.However, the code readability could be improved by extracting the allocation into a separate function.
Consider extracting the allocation into a separate function to improve readability:
- m_query = new (std::nothrow) - Query(search_time_lower_bound, - search_time_upper_bound, - wildcard_queries, - search_time_termination_margin); + m_query = create_query( + search_time_lower_bound, + search_time_upper_bound, + wildcard_queries, + search_time_termination_margin);Add this helper function:
private: static auto create_query( clp::ir::epoch_time_ms_t search_time_lower_bound, clp::ir::epoch_time_ms_t search_time_upper_bound, std::vector<WildcardQuery> const& wildcard_queries, clp::ir::epoch_time_ms_t search_time_termination_margin ) -> Query* { return new (std::nothrow) Query( search_time_lower_bound, search_time_upper_bound, wildcard_queries, search_time_termination_margin); }src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
646-647: LGTM: Proper use ofstd::nothrowfor memory allocationThe change correctly implements exception-free memory allocation with proper null pointer checking. This fixes the issue #103 by allowing proper handling of out-of-memory conditions.
Consider making the error message more specific:
- get_c_str_from_constexpr_string_view(clp_ffi_py::cOutOfMemoryError) + "Failed to allocate memory for KeyValuePairLogEvent"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/clp_ffi_py/ir/native/DeserializerBufferReader.cpp(2 hunks)src/clp_ffi_py/ir/native/PyDeserializer.cpp(4 hunks)src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp(2 hunks)src/clp_ffi_py/ir/native/PyLogEvent.cpp(2 hunks)src/clp_ffi_py/ir/native/PyMetadata.cpp(3 hunks)src/clp_ffi_py/ir/native/PyQuery.cpp(2 hunks)src/clp_ffi_py/ir/native/PySerializer.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/clp_ffi_py/ir/native/DeserializerBufferReader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PyDeserializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PyMetadata.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PyQuery.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PyLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
📓 Learnings (4)
src/clp_ffi_py/ir/native/DeserializerBufferReader.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#89
File: src/clp_ffi_py/ir/native/DeserializerBufferReader.hpp:57-58
Timestamp: 2024-11-16T03:46:52.844Z
Learning: In the `DeserializerBufferReader` class (`src/clp_ffi_py/ir/native/DeserializerBufferReader.hpp`), the `try_seek_from_begin` method should only be implemented when there is a specific use case requiring seek functionality.
src/clp_ffi_py/ir/native/PyDeserializer.cpp (4)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#90
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp:653-675
Timestamp: 2024-11-16T03:42:40.523Z
Learning: In `src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp`, within the function `PyKeyValuePairLogEvent::to_dict`, only `clp::TraceableException` needs to be caught, as no other `std::exception`s can be thrown.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#94
File: src/clp_ffi_py/ir/native/PyDeserializer.cpp:218-242
Timestamp: 2024-11-23T05:55:57.106Z
Learning: In `src/clp_ffi_py/ir/native/PyDeserializer.cpp`, the `deserialize_log_event` method in the `PyDeserializer` class ensures proper loop termination and does not risk infinite loops, even when handling incomplete or corrupted streams.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#94
File: src/clp_ffi_py/ir/native/PyDeserializer.cpp:206-208
Timestamp: 2024-11-23T05:55:08.019Z
Learning: In code interfacing with CPython (e.g., `src/clp_ffi_py/ir/native/PyDeserializer.cpp`), using raw pointers for memory management is acceptable due to CPython's memory management requirements.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#94
File: src/clp_ffi_py/ir/native/PyDeserializer.hpp:246-248
Timestamp: 2024-11-23T05:54:37.767Z
Learning: In the `src/clp_ffi_py/ir/native` module, raw pointers with `gsl::owner` are intentionally used instead of `std::unique_ptr` to comply with CPython's memory management requirements.
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#83
File: src/clp_ffi_py/ir/native/PySerializer.hpp:73-76
Timestamp: 2024-11-19T01:13:58.505Z
Learning: In the `PySerializer` class in `src/clp_ffi_py/ir/native/PySerializer.hpp`, the `close_serializer()` method never fails, so additional error handling in the `clean()` method is unnecessary.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#81
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp:0-0
Timestamp: 2024-11-14T06:30:18.577Z
Learning: In `PyKeyValuePairLogEvent`, we assume invalid objects will never be created, so null pointer checks in methods like `get_kv_pair_log_event()` are unnecessary, assuming CPython system runs correctly.
Learnt from: LinZhihao-723
PR: y-scope/clp-ffi-py#90
File: src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp:653-675
Timestamp: 2024-11-16T03:42:40.523Z
Learning: In `src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp`, within the function `PyKeyValuePairLogEvent::to_dict`, only `clp::TraceableException` needs to be caught, as no other `std::exception`s can be thrown.
🔇 Additional comments (7)
src/clp_ffi_py/ir/native/DeserializerBufferReader.cpp (1)
30-39: LGTM! Proper implementation of memory allocation failure handling.
The implementation correctly uses std::nothrow and handles allocation failures by setting an appropriate error message, which aligns with the PR objectives.
src/clp_ffi_py/ir/native/PyMetadata.cpp (2)
Line range hint 233-240: LGTM! Consistent error handling implementation.
The implementation correctly uses std::nothrow and follows the established pattern for handling allocation failures.
247-254: LGTM! Proper error handling within exception handling context.
The implementation correctly handles memory allocation failures while maintaining proper exception handling context.
src/clp_ffi_py/ir/native/PyDeserializer.cpp (1)
205-215: LGTM! Proper handling of complex type allocation.
The implementation correctly uses std::nothrow with template types and maintains consistent error handling patterns.
src/clp_ffi_py/ir/native/PyLogEvent.cpp (1)
Line range hint 499-507: LGTM! Proper handling of memory allocation failure.
The implementation correctly uses new (std::nothrow) and checks for allocation failure, setting an appropriate Python runtime error when allocation fails. This change aligns with the PR objectives to improve memory allocation failure handling.
src/clp_ffi_py/ir/native/PySerializer.cpp (1)
Line range hint 405-412: LGTM! Proper handling of memory allocation failure.
The implementation correctly uses new (std::nothrow) and checks for allocation failure, setting an appropriate Python runtime error when allocation fails. This change aligns with the PR objectives to improve memory allocation failure handling.
src/clp_ffi_py/ir/native/PyKeyValuePairLogEvent.cpp (1)
7-7: LGTM: Required header for std::nothrow
The addition of the <new> header is necessary for using std::nothrow in memory allocations.
| = new (std::nothrow) clp::ffi::KeyValuePairLogEvent{std::move(log_event)}; | ||
| if (nullptr == m_deserialized_log_event) { | ||
| // TODO: Set this to a proper error code when user-defined error code is supported. | ||
| return IRErrorCode::IRErrorCode_Eof; |
There was a problem hiding this comment.
maybe we should return IRErrorCode::NoMem.
Unfortuately it will require updating clp repo so obviously out of scope of this PR, lol
There was a problem hiding this comment.
Sigh, correct. I'm trying to proceed error handling improvements. Hopefully we will have the user-defined user code supported soon.
Description
This PR fixes #103.
For all
newoperators, we now usestd::nothrowto explicitly disable exception throwing. As a result, we need to check manually whether the returned pointer is null and set out-of-memory errors accordingly.Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Documentation