Added SSL read/write queues. #952
Conversation
SSL connection was unstable on high load
|
Some fuzz errors |
|
Do you have an extension or new ACE unit test to validate this new code? |
No. The existing test should continue to work. But will only test with one message in the queue. Hence the FIFO logic is not validated by test cases. |
|
Can you extend the exiting test to also test the FIFO logic? |
WalkthroughThe changes update asynchronous stream handling by replacing single-pointer mechanisms with queues for managing multiple asynchronous read and write results. In both implementation and declarations, pointers have been substituted with container types (queue/list) to store multiple results. The constructor, destructor, read/write methods, and notification functions have been modified to support queue-based management and proper memory cleanup. These modifications improve concurrent operation handling and lifecycle management of asynchronous operations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Handler
participant S as SSL_Asynch_Stream
participant RQ as Read Result Queue
U->>S: Call read()
S->>RQ: Enqueue new Read Result
Note over S,RQ: Asynchronous read operation in progress
S->>RQ: On completion, dequeue result
RQ--)S: Return read result
S->>U: notify_read(result)
sequenceDiagram
participant U as User Handler
participant S as SSL_Asynch_Stream
participant WQ as Write Result Queue
U->>S: Call write(data)
S->>WQ: Enqueue new Write Result
Note over S,WQ: Asynchronous write operation in progress
S->>WQ: On completion, dequeue result
WQ--)S: Return write result
S->>U: notify_write(result)
Poem
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (4)
ACE/ace/SSL/SSL_Asynch_Stream.cpp (2)
340-354: Prefer using smart pointers for Result objects.
Creating raw pointers and manually deleting them in multiple places can be error-prone. Using smart pointers (e.g.,std::unique_ptr) could reduce memory management risks and simplify the code.-ACE_SSL_Asynch_Read_Stream_Result* result = 0; -ACE_NEW_RETURN (result, - ACE_SSL_Asynch_Read_Stream_Result ( - ... - ), - -1); -ext_read_result_queue_.push_back(result); +auto result = std::make_unique<ACE_SSL_Asynch_Read_Stream_Result>( + *this->ext_handler_, + this->handle (), + message_block, + bytes_to_read, + act, + this->proactor_->get_handle(), + priority, + signal_number +); // ... +ext_read_result_queue_.push_back(result.release());
380-395: Unify read/write creation logic.
The logic creating and queuing write results mirrors the read results. Consider extracting common code into a helper function for consistency and reduced duplication.ACE/ace/SSL/SSL_Asynch_Stream.h (2)
16-16: Confirm container choice for better performance.
Usingstd::listfor frequent front pop and back push is acceptable, butstd::dequemay offer better cache locality. Evaluate if switching tostd::dequewould help.
383-389: Raw pointer storage in container.
Storing raw pointers in a standard container requires diligent manual memory handling. If feasible, consider a container of smart pointers (e.g.,std::unique_ptr) to reduce the chance of leaks or double frees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ACE/ace/SSL/SSL_Asynch_Stream.cpp(11 hunks)ACE/ace/SSL/SSL_Asynch_Stream.h(2 hunks)
🔇 Additional comments (7)
ACE/ace/SSL/SSL_Asynch_Stream.cpp (7)
152-159: Consider verifying no active requests before destruction.
Although this destructor attempts to delete all queued results, please ensure all in-flight read/write operations have completed or been canceled before these pointers are destroyed. Otherwise, there's a risk of dangling pointers if completions arrive after destruction.
566-566: Conditional check for an empty read queue looks correct.
No issues here; the code properly returns early if no read operations are queued.
577-579: Verify front() usage under concurrency.
Accessingfront()is valid only if the queue remains non-empty. While the preceding check ensures emptiness is handled, consider re-checking if concurrency might invalidate that assumption.
628-628: Early return for empty write queue is clear.
This condition matches the read queue approach and maintains consistency.
639-640: Front-access usage mirrors do_SSL_read.
Just like in do_SSL_read(), the code retrieves the front item for processing. Implementation looks consistent with the empty-check.
730-741: Confirm multiple queued reads are fully processed.
This notification logic handles completion for the front of the queue, popping it on success. Verify that subsequent queued reads are also eventually processed—especially under partial reads or repeated SSL states.
760-772: Write completion notification is consistent.
This notification pattern aligns with the read side. The approach appears sound for single-queue usage per completion.
Comments in the code states below:
// only one read operation is allowed now
// later it will be possible to make a queue
Somehow our usage of ACE 6.1.7 hits this limitation in some situations. Resulting in read/write errors.
Adding read/write queues resolves this limitation.
Summary by CodeRabbit
Refactor
Chores