Updating channel to be mpsc#83
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the async I/O “channel” implementation to behave as a multi-producer/single-consumer (MPSC) channel with explicit close semantics, and adds tests to validate multi-producer behavior (including cross-thread sends).
Changes:
- Add mutex-protected state to
mpsc_channel_subscriptionand introduceclose()to wake a blocked receiver and signal end-of-stream. - Extend
mpsc_channel_wstreamwith an asyncclose()API and add static-assert coverage for the closeable-stream concept. - Add new unit tests covering multi-producer message delivery and thread-pool-based producers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
include/webcraft/async/io/core.hpp |
Makes MPSC channel state thread-safe, adds close support, and updates awaitable behavior to return std::optional<T>. |
tests/src/test_async_io_core.cpp |
Adds new tests for multi-producer and thread-safe MPSC channel usage, plus required includes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor to use std::move for efficiency in value handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto run_producers_and_close = [&]() -> task<void> | ||
| { | ||
| std::vector<task<void>> producers; | ||
|
|
||
| std::vector<std::string> messages{expected_messages.begin(), expected_messages.end()}; | ||
|
|
||
| for (const auto &message : messages) | ||
| { | ||
| producers.push_back(producer_one(message)); | ||
| } |
There was a problem hiding this comment.
run_producers_and_close builds messages from expected_messages while consumer_fn concurrently erases from expected_messages. This is an unsynchronized cross-task access to the same std::unordered_set, which is undefined behavior (iterator invalidation / data race). Generate messages from an independent source (e.g., the same iota range) or build messages/expected_messages completely before starting the consumer task.
| auto producer_one = [writer, &pool](std::string message1) mutable -> task<void> | ||
| { | ||
| co_await threaded_awaitable{pool}; | ||
| auto check = co_await writer.send(message1); |
There was a problem hiding this comment.
In producer_one, writer.send(message1) passes an lvalue std::string, causing an extra copy into send(T val) (and then a move). Use std::move(message1) when calling send to avoid the redundant copy, especially since this test sends 1000 messages.
| auto check = co_await writer.send(message1); | |
| auto check = co_await writer.send(std::move(message1)); |
| sync_wait(task_fn()); | ||
| } | ||
|
|
||
| TEST_CASE(MpScChannel) |
There was a problem hiding this comment.
Test case name MpScChannel looks like a typo/inconsistent acronym casing for an MPSC channel. Consider renaming to something unambiguous like MpscChannel/MPSCChannel to match the channel type (make_mpsc_channel).
| TEST_CASE(MpScChannel) | |
| TEST_CASE(MpscChannel) |
No description provided.