tests and more #34
tests and more #34
Conversation
📝 WalkthroughWalkthroughAdds an io_stream impl accessor; requires explicit tls::context for TLS streams; implements a concrete tls::context API; adds socket-pair test utilities and many TLS/socket tests; refactors socket/acceptor internals to wrapper-based shared lifetimes; centralizes OS error mapping via make_err and standardizes empty-buffer semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test harness
participant IO as io_context
participant Acceptor as Acceptor
participant SocketA as Socket A
participant SocketB as Socket B
Test->>Acceptor: bind(listen on loopback:port)
Test->>IO: async_accept(SocketA)
Test->>IO: async_connect(SocketB)
IO->>Acceptor: accept incoming -> create peer wrapper/internal
IO->>SocketB: complete connect
Acceptor->>SocketA: accept completes (assign wrapper + internal)
IO->>Test: both operations complete -> return (SocketA, SocketB)
sequenceDiagram
participant ClientApp as Client
participant CtxC as Client tls::context
participant IO as io_context
participant CStream as Client TLS stream
participant SStream as Server TLS stream
participant CtxS as Server tls::context
ClientApp->>CtxC: make_client_context()
ServerApp->>CtxS: make_server_context()
ClientApp->>CStream: construct(io_stream, CtxC)
ServerApp->>SStream: construct(io_stream, CtxS)
ClientApp->>IO: async_handshake(CStream)
ServerApp->>IO: async_handshake(SStream)
CStream->>SStream: TLS handshake exchange via IO
IO->>ClientApp: client handshake complete
IO->>ServerApp: server handshake complete
ClientApp->>CStream: async_write("hello")
CStream->>SStream: encrypted data delivered -> decrypt
ServerApp->>SStream: async_read -> "hello"
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
An automated preview of the documentation is available at https://34.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-20 16:38:05 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wolfssl/src/wolfssl_stream.cpp (1)
929-938: Silent failure on init_ssl error may cause issues downstream.When
init_ssl()fails, the constructor silently returns withimpl_unset (nullptr). Callers have no way to detect this failure, which could lead to null pointer dereference or unexpected behavior when using the stream.🔧 Consider throwing an exception or providing a validity check
Option 1: Throw on failure
auto ec = impl->init_ssl(); if( ec ) { delete impl; - // For now, silently fail - could throw or store error - return; + throw system::system_error( ec ); }Option 2: Add
is_valid()method and document the need to check:bool wolfssl_stream::is_valid() const noexcept { return impl_ != nullptr; }src/openssl/src/openssl_stream.cpp (1)
756-764: Same silent failure pattern as WolfSSL stream.This has the same issue noted in
wolfssl_stream.cpp- silent failure oninit_ssl()error leavesimpl_unset with no way for callers to detect the failure.
🤖 Fix all issues with AI agents
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 185-191: The call to SSL_CTX_set_cipher_list does not check its
return value so invalid cipher strings go unnoticed; update the block that
handles cd.ciphersuites (where SSL_CTX_set_security_level(ctx_, 0) and
SSL_CTX_set_cipher_list(ctx_, cd.ciphersuites.c_str()) are called) to verify the
return value of SSL_CTX_set_cipher_list, and on failure log an error (including
cd.ciphersuites and any OpenSSL error via ERR_get_error()/ERR_error_string) and
abort initialization or throw an exception so the caller knows cipher
configuration failed.
In `@test/unit/CMakeLists.txt`:
- Around line 27-35: The CMake if-statement is using the wrong OpenSSL variable
name; change the conditional from OpenSSL_FOUND to the standard OPENSSL_FOUND so
the block that links boost_corosio_openssl and sets BOOST_COROSIO_HAS_OPENSSL=1
actually runs when OpenSSL is found; update the if(...) check that surrounds
target_link_libraries(boost_corosio_tests PRIVATE boost_corosio_openssl) and
target_compile_definitions(boost_corosio_tests PRIVATE
BOOST_COROSIO_HAS_OPENSSL=1) to use OPENSSL_FOUND.
In `@test/unit/tls/test_utils.hpp`:
- Around line 96-114: The wrong_ca_cert_pem constant contains bogus placeholder
PEM data and must be replaced with a real self-signed CA certificate distinct
from server_cert_pem so parsing succeeds; update the inline constexpr char
const* wrong_ca_cert_pem to a valid PEM-encoded self-signed cert (different from
server_cert_pem) so add_certificate_authority() can load it and tests exercise
CA verification failure rather than failing during context setup.
🧹 Nitpick comments (7)
src/corosio/src/test/socket_pair.cpp (2)
26-36: Consider thread safety for parallel test execution.The static
next_test_portvariable is accessed without synchronization. If tests run in parallel threads, this creates a data race. While the port cycling provides some mitigation against port conflicts, consider usingstd::atomic<std::uint16_t>if parallel test execution is expected.♻️ Suggested fix for thread safety
-std::uint16_t next_test_port = 0; +std::atomic<std::uint16_t> next_test_port{0}; std::uint16_t get_test_port() noexcept { - auto port = test_port_base + (next_test_port % test_port_range); - ++next_test_port; + auto idx = next_test_port.fetch_add(1, std::memory_order_relaxed); + auto port = test_port_base + (idx % test_port_range); return static_cast<std::uint16_t>(port); }
80-91: Inconsistent resource cleanup in error paths.The error paths close
accand conditionallys1, buts2(which was opened at line 56) is not explicitly closed before throwing. While the socket destructor likely handles cleanup, explicit cleanup would be more consistent with the established pattern.♻️ Suggested fix for consistent cleanup
if (!accept_done || accept_ec) { acc.close(); + s2.close(); throw std::runtime_error("socket_pair accept failed"); } if (!connect_done || connect_ec) { acc.close(); s1.close(); + s2.close(); throw std::runtime_error("socket_pair connect failed"); }src/corosio/src/tls/context.cpp (2)
52-61: Consider usingstd::ios::ateto pre-allocate string size.The current pattern reads the entire file through
std::ostringstream, which may cause multiple reallocations for large certificate files. Consider pre-sizing for efficiency.♻️ Optional: Pre-allocate string capacity
system::result<void> context:: use_certificate_file( std::string_view filename, file_format format ) { - std::ifstream file( std::string( filename ), std::ios::binary ); + std::ifstream file( std::string( filename ), std::ios::binary | std::ios::ate ); if( !file ) return system::error_code( ENOENT, system::generic_category() ); - std::ostringstream ss; - ss << file.rdbuf(); - impl_->entity_certificate = ss.str(); + auto size = file.tellg(); + file.seekg( 0 ); + std::string data( static_cast<std::size_t>( size ), '\0' ); + file.read( data.data(), size ); + impl_->entity_certificate = std::move( data ); impl_->entity_cert_format = format; return {}; }
113-131: PKCS#12 stubs are acceptable with TODO markers.The unimplemented PKCS#12 methods correctly return
ENOTSUPand have clear TODO comments. Consider tracking these in an issue for future implementation.Would you like me to open an issue to track PKCS#12 implementation?
test/unit/tls/cross_ssl_stream.cpp (1)
158-171: Disabled failure tests are well-documented.The comment clearly explains the technical reason (socket cancellation doesn't propagate to TLS handshake state machines). Consider tracking the TLS-aware cancellation feature as a future enhancement.
Would you like me to open an issue to track implementing TLS-aware cancellation so these failure tests can be enabled?
test/unit/tls/test_utils.hpp (2)
121-169: Assert context configuration results to surface bad test assets early.
These helpers ignoreboost::system::result<void>from context mutators; checking them makes failures explicit and avoids chasing handshake errors.Suggested pattern (apply to the helpers below)
inline context make_server_context() { context ctx; - ctx.use_certificate( server_cert_pem, file_format::pem ); - ctx.use_private_key( server_key_pem, file_format::pem ); - ctx.set_verify_mode( verify_mode::none ); + BOOST_TEST( ctx.use_certificate( server_cert_pem, file_format::pem ) ); + BOOST_TEST( ctx.use_private_key( server_key_pem, file_format::pem ) ); + BOOST_TEST( ctx.set_verify_mode( verify_mode::none ) ); return ctx; }
373-439: Cancel the timeout once both handshakes complete to avoid fixed 500ms waits.
As written,ioc.run()always waits for the timer even when both sides finish quickly; you already trackclient_done/server_done, so you can cancel the timer early.One way to cancel early
- // Concurrent handshakes (at least one should fail) - capy::run_async( ioc.get_executor() )( - [&client, &client_failed, &client_done]() -> capy::task<> - { - auto [ec] = co_await client.handshake( tls_stream::client ); - if( ec ) - client_failed = true; - client_done = true; - }() ); - - capy::run_async( ioc.get_executor() )( - [&server, &server_failed, &server_done]() -> capy::task<> - { - auto [ec] = co_await server.handshake( tls_stream::server ); - if( ec ) - server_failed = true; - server_done = true; - }() ); - - // Timer to unblock stuck handshakes - when one side fails, the other - // may block waiting for data. Timer cancels socket operations to unblock them. - timer timeout( ioc ); - timeout.expires_after( std::chrono::milliseconds( 500 ) ); - capy::run_async( ioc.get_executor() )( - [&timeout, &s1, &s2, &client_done, &server_done]() -> capy::task<> - { - auto [ec] = co_await timeout.wait(); - if( !ec ) - { - // Timer expired - cancel pending operations then close sockets - s1.cancel(); - s2.cancel(); - s1.close(); - s2.close(); - } - }() ); + // Timer to unblock stuck handshakes - when one side fails, the other + // may block waiting for data. Timer cancels socket operations to unblock them. + timer timeout( ioc ); + timeout.expires_after( std::chrono::milliseconds( 500 ) ); + auto maybe_cancel = [&]() + { + if( client_done && server_done ) + timeout.cancel(); + }; + + // Concurrent handshakes (at least one should fail) + capy::run_async( ioc.get_executor() )( + [&client, &client_failed, &client_done, &maybe_cancel]() -> capy::task<> + { + auto [ec] = co_await client.handshake( tls_stream::client ); + if( ec ) + client_failed = true; + client_done = true; + maybe_cancel(); + }() ); + + capy::run_async( ioc.get_executor() )( + [&server, &server_failed, &server_done, &maybe_cancel]() -> capy::task<> + { + auto [ec] = co_await server.handshake( tls_stream::server ); + if( ec ) + server_failed = true; + server_done = true; + maybe_cancel(); + }() ); + + capy::run_async( ioc.get_executor() )( + [&timeout, &s1, &s2]() -> capy::task<> + { + auto [ec] = co_await timeout.wait(); + if( !ec ) + { + // Timer expired - cancel pending operations then close sockets + s1.cancel(); + s2.cancel(); + s1.close(); + s2.close(); + } + }() );
|
|
||
| // Apply cipher suites if provided | ||
| if( !cd.ciphersuites.empty() ) | ||
| { | ||
| SSL_CTX_set_security_level( ctx_, 0 ); | ||
| SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() ); | ||
| } |
There was a problem hiding this comment.
Missing error check on SSL_CTX_set_cipher_list.
SSL_CTX_set_cipher_list() returns 1 on success and 0 if no valid ciphers are found. The return value should be checked to detect invalid cipher strings.
🔧 Proposed fix
// Apply cipher suites if provided
if( !cd.ciphersuites.empty() )
{
SSL_CTX_set_security_level( ctx_, 0 );
- SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() );
+ if( SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() ) != 1 )
+ {
+ SSL_CTX_free( ctx_ );
+ ctx_ = nullptr;
+ return;
+ }
}🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 185 - 191, The call to
SSL_CTX_set_cipher_list does not check its return value so invalid cipher
strings go unnoticed; update the block that handles cd.ciphersuites (where
SSL_CTX_set_security_level(ctx_, 0) and SSL_CTX_set_cipher_list(ctx_,
cd.ciphersuites.c_str()) are called) to verify the return value of
SSL_CTX_set_cipher_list, and on failure log an error (including cd.ciphersuites
and any OpenSSL error via ERR_get_error()/ERR_error_string) and abort
initialization or throw an exception so the caller knows cipher configuration
failed.
| if (WolfSSL_FOUND) | ||
| target_link_libraries(boost_corosio_tests PRIVATE boost_corosio_wolfssl) | ||
| target_compile_definitions(boost_corosio_tests PRIVATE BOOST_COROSIO_HAS_WOLFSSL=1) | ||
| endif() | ||
|
|
||
| if (OpenSSL_FOUND) | ||
| target_link_libraries(boost_corosio_tests PRIVATE boost_corosio_openssl) | ||
| target_compile_definitions(boost_corosio_tests PRIVATE BOOST_COROSIO_HAS_OPENSSL=1) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for CMake configuration related to OpenSSL and WolfSSL
rg -n "find_package.*OpenSSL|find_package.*WolfSSL|OpenSSL_FOUND|OPENSSL_FOUND|WolfSSL_FOUND|WOLFSSL_FOUND" --type cmake -A 2Repository: cppalliance/corosio
Length of output: 1652
🏁 Script executed:
# Also check for target definitions
rg -n "boost_corosio_wolfssl|boost_corosio_openssl|add_library.*openssl|add_library.*wolfssl" --type cmake -A 2Repository: cppalliance/corosio
Length of output: 1914
🏁 Script executed:
# Check the structure of CMake files in the project
git ls-files | grep -i cmake$ | head -20Repository: cppalliance/corosio
Length of output: 86
🌐 Web query:
CMake FindOpenSSL module OpenSSL_FOUND variable name
💡 Result:
The FindOpenSSL module sets the variable named OPENSSL_FOUND (all uppercase). [1]
Sources:
- CMake FindOpenSSL documentation (module result variables). [1] https://cmake.org/cmake/help/latest/module/FindOpenSSL.html
🏁 Script executed:
# Search for any OpenSSL-related CMake files
find . -name "*OpenSSL*" -o -name "*openssl*" | grep -i cmakeRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check the find_package call for OpenSSL to see if it specifies CONFIG mode
rg -n "find_package.*OpenSSL" --type cmake -A 3Repository: cppalliance/corosio
Length of output: 324
Fix CMake variable name for OpenSSL detection.
Change OpenSSL_FOUND to OPENSSL_FOUND in CMakeLists.txt:217 and test/unit/CMakeLists.txt:32. The standard CMake FindOpenSSL module sets OPENSSL_FOUND (all uppercase), not OpenSSL_FOUND. Using the incorrect variable name will cause the OpenSSL configuration and test linking to be silently skipped whenever the package is found.
🤖 Prompt for AI Agents
In `@test/unit/CMakeLists.txt` around lines 27 - 35, The CMake if-statement is
using the wrong OpenSSL variable name; change the conditional from OpenSSL_FOUND
to the standard OPENSSL_FOUND so the block that links boost_corosio_openssl and
sets BOOST_COROSIO_HAS_OPENSSL=1 actually runs when OpenSSL is found; update the
if(...) check that surrounds target_link_libraries(boost_corosio_tests PRIVATE
boost_corosio_openssl) and target_compile_definitions(boost_corosio_tests
PRIVATE BOOST_COROSIO_HAS_OPENSSL=1) to use OPENSSL_FOUND.
| // Different self-signed CA for "wrong CA" test scenarios | ||
| // (A different self-signed cert that won't verify server_cert_pem) | ||
| inline constexpr char const* wrong_ca_cert_pem = | ||
| "-----BEGIN CERTIFICATE-----\n" | ||
| "MIICpDCCAYwCCQDU+pQ4P0jwoDANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls\n" | ||
| "b2NhbGhvc3QwHhcNMjMwMTAxMDAwMDAwWhcNMzMwMTAxMDAwMDAwWjAUMRIwEAYD\n" | ||
| "VQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC7\n" | ||
| "o5e7Xv5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z\n" | ||
| "5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z\n" | ||
| "5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z\n" | ||
| "5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z\n" | ||
| "5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z5Z\n" | ||
| "5Z5Z5Z5ZAgMBAAEwDQYJKoZIhvcNAQELBQADggEBADummy0000000000000000000\n" | ||
| "0000000000000000000000000000000000000000000000000000000000000000000\n" | ||
| "0000000000000000000000000000000000000000000000000000000000000000000\n" | ||
| "0000000000000000000000000000000000000000000000000000000000000000000\n" | ||
| "0000000000000000000000000000000000000000000000000000000000000000000\n" | ||
| "0000000000000000000000000000000000000000000000=\n" | ||
| "-----END CERTIFICATE-----\n"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/unit/tls && head -150 test_utils.hpp | tail -70Repository: cppalliance/corosio
Length of output: 3299
🏁 Script executed:
# Search for the wrong_ca_cert_pem usage in tests
rg "wrong_ca_cert_pem" --type cpp -B 2 -A 5Repository: cppalliance/corosio
Length of output: 1260
🏁 Script executed:
# Check if there are other CA certificates defined in the same file for comparison
rg "cert_pem|key_pem" test/unit/tls/test_utils.hpp | head -20Repository: cppalliance/corosio
Length of output: 624
Replace wrong_ca_cert_pem with a valid self-signed PEM certificate.
The certificate data contains placeholder content: the public key section has repeating Z characters, and the signature has a Dummy prefix with repeating 0s. This will fail to parse when add_certificate_authority() attempts to load it, causing CA verification tests to fail during context setup rather than during the intended verification logic. Use a real self-signed CA certificate distinct from server_cert_pem to ensure the "wrong CA" scenario actually tests the intended rejection pathway.
🤖 Prompt for AI Agents
In `@test/unit/tls/test_utils.hpp` around lines 96 - 114, The wrong_ca_cert_pem
constant contains bogus placeholder PEM data and must be replaced with a real
self-signed CA certificate distinct from server_cert_pem so parsing succeeds;
update the inline constexpr char const* wrong_ca_cert_pem to a valid PEM-encoded
self-signed cert (different from server_cert_pem) so add_certificate_authority()
can load it and tests exercise CA verification failure rather than failing
during context setup.
- Add socket_pair.hpp header declaring make_socket_pair function - Add socket_pair.cpp implementation using loopback TCP connection - Add unit tests for socket pair creation and bidirectional I/O
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@include/boost/corosio/read.hpp`:
- Around line 79-84: The docblock for the template function read(io_stream& ios,
MB const& bs) uses an outdated `@param` name "buffers"; update the `@param` entry to
use "bs" to match the parameter in the function signature
(template<capy::MutableBufferSequence MB> capy::task<io_result<std::size_t>>
read(io_stream& ios, MB const& bs)) so Doxygen stays in sync.
In `@test/unit/socket.cpp`:
- Around line 619-645: In testReadString() the socket `s1` (aliased to `a`
inside the coroutine) is closed inside the coroutine and then closed again after
ioc.run() causing a double-close; fix by removing the redundant post-run
`s1.close()` (or replace it with a conditional/is_open check) so only `s2` (or
sockets that weren't closed in the coroutine) are closed outside the coroutine;
update references in testReadString(), and keep the coroutine parameters
`socket& a, socket& b` and the existing close call inside the coroutine intact.
- Around line 647-674: The test testReadPartialEOF double-closes the same
socket: the coroutine closes 'a' (the s1 socket) via a.close(), and then the
test calls s1.close() again after ioc.run(); remove the redundant s1.close() (or
conditionally skip closing s1) so the socket is only closed once—update the
cleanup at the end of testReadPartialEOF to only close s2 (or neither if both
were closed in the coroutine).
In `@test/unit/tls/cross_ssl_stream.cpp`:
- Line 10: Remove the local Windows development path reference found in the
comment on cross_ssl_stream.cpp (the line containing
"c:\Users\Vinnie\.cursor\plans\tls_stream_tests_83c24f98.plan.md"); delete that
hard-coded path or replace it with a repository-relative or generic reference
(e.g., remove the .cursor path or replace with a neutral
"plans/tls_stream_tests.plan" or a short TODO) so no user-specific/local
filesystem paths remain in test/unit/tls/cross_ssl_stream.cpp.
In `@test/unit/tls/test_utils.hpp`:
- Around line 210-239: The test_stream helper assumes read_some returns the full
5 bytes which can be flaky; change the two reads that call b.read_some and
a.read_some to accumulate into buf in a loop until the expected number of bytes
(5) is received, checking the error (ec) on each iteration and advancing the
buffer pointer/count accordingly; keep the existing write_some calls and
assertions for their returned n values, but replace the single-read assertions
for ec and n with assertions after the loop that the total bytes read equals 5
and that the concatenated data equals the expected string ("hello" and "world");
this touches the test_stream function and the calls to io_stream::read_some.
- Around line 65-94: Add a .gitleaksignore allowlist entry to prevent false
positives for the embedded Boost.Beast test credentials: identify the constant
server_key_pem in test_utils.hpp and whitelist that file (or a pattern matching
this test file) in .gitleaksignore with a brief justification mentioning the key
is an intentionally embedded test certificate from Boost.Beast.
♻️ Duplicate comments (2)
test/unit/tls/test_utils.hpp (1)
96-114: Replace placeholderwrong_ca_cert_pemwith a valid CA cert.
Line 99–113 still looks like placeholder data, so TLS backends may fail to parse or silently skip it, meaning the “wrong CA” path isn’t really exercised. Use a real self‑signed CA cert distinct fromserver_cert_pem.src/openssl/src/openssl_stream.cpp (1)
186-191: CheckSSL_CTX_set_cipher_list()return value.
Line 190 ignores the return code; invalid cipher strings will fail silently.🔧 Suggested fix
if( !cd.ciphersuites.empty() ) { SSL_CTX_set_security_level( ctx_, 0 ); - SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() ); + if( SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() ) != 1 ) + { + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + } }
🧹 Nitpick comments (4)
include/boost/corosio/io_stream.hpp (1)
220-231: Consider const/non-const overloads forget_impl().
Line 227 returns a mutable pointer from aconstmethod, which permits mutation through aconst io_stream. If this is meant for testing but you want to preserve const‑correctness, split into const and non‑const overloads.♻️ Suggested adjustment
- io_stream_impl* - get_impl() const noexcept - { - return static_cast<io_stream_impl*>(impl_); - } + io_stream_impl* + get_impl() noexcept + { + return static_cast<io_stream_impl*>(impl_); + } + + io_stream_impl const* + get_impl() const noexcept + { + return static_cast<io_stream_impl const*>(impl_); + }src/corosio/src/tls/context.cpp (1)
113-131: PKCS#12 TODOs: consider tracking or documenting ENOTSUP.
If these are intentionally deferred, consider a tracked issue or doc note so callers aren’t surprised. I can help wire this up if desired.test/unit/tls/wolfssl_stream.cpp (1)
84-89: Consider tracking the disabled failure tests with an issue/TODO.
This helps ensure the cancellation behavior gets revisited.test/unit/tls/cross_ssl_stream.cpp (1)
140-144: Clarify the intent ofset_ciphersuites("").Setting an empty cipher suite string on an anonymous context is unusual. If the intent is to force the server to have no valid ciphers (causing handshake failure), this works but could benefit from a brief inline comment explaining the intent. If the goal is something else, this may not behave as expected.
Suggested clarification
{ auto client_ctx = make_client_context(); auto server_ctx = make_anon_context(); + // Clear ciphersuites to force handshake failure - server has no usable ciphers server_ctx.set_ciphersuites( "" ); run_tls_test_fail( ioc, client_ctx, server_ctx, make_openssl, make_wolfssl ); ioc.restart(); }
| template<capy::MutableBufferSequence MB> | ||
| capy::task<io_result<std::size_t>> | ||
| read(io_stream& ios, MutableBufferSequence const& buffers) | ||
| read(io_stream& ios, MB const& bs) | ||
| { | ||
| consuming_buffers<MutableBufferSequence> consuming(buffers); | ||
| std::size_t const total_size = capy::buffer_size(buffers); | ||
| consuming_buffers<MB> consuming(bs); | ||
| std::size_t const total_size = capy::buffer_size(bs); |
There was a problem hiding this comment.
Update the @param name to match bs.
The parameter was renamed to bs (Line 81), but the doc block still uses buffers, which can desync Doxygen-style docs.
📚 Suggested doc fix
- `@param` buffers The buffer sequence to read data into.
+ `@param` bs The buffer sequence to read data into.🤖 Prompt for AI Agents
In `@include/boost/corosio/read.hpp` around lines 79 - 84, The docblock for the
template function read(io_stream& ios, MB const& bs) uses an outdated `@param`
name "buffers"; update the `@param` entry to use "bs" to match the parameter in
the function signature (template<capy::MutableBufferSequence MB>
capy::task<io_result<std::size_t>> read(io_stream& ios, MB const& bs)) so
Doxygen stays in sync.
| void | ||
| testReadString() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| std::string send_data = "Hello, this is a test message!"; | ||
| co_await write(a, capy::const_buffer( | ||
| send_data.data(), send_data.size())); | ||
| a.close(); | ||
|
|
||
| // Read into string until EOF | ||
| std::string result; | ||
| auto [ec, n] = co_await read(b, result); | ||
| // EOF is expected | ||
| BOOST_TEST(ec == capy::error::eof); | ||
| BOOST_TEST_EQ(n, send_data.size()); | ||
| BOOST_TEST_EQ(result, send_data); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
Double-close on s1.
Similar to testReadAfterPeerClose, socket a (aliased as s1) is closed at line 631 inside the coroutine, then s1.close() is called again at line 643.
Suggested fix
ioc.run();
- s1.close();
+ // s1 already closed in coroutine
s2.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| testReadString() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| std::string send_data = "Hello, this is a test message!"; | |
| co_await write(a, capy::const_buffer( | |
| send_data.data(), send_data.size())); | |
| a.close(); | |
| // Read into string until EOF | |
| std::string result; | |
| auto [ec, n] = co_await read(b, result); | |
| // EOF is expected | |
| BOOST_TEST(ec == capy::error::eof); | |
| BOOST_TEST_EQ(n, send_data.size()); | |
| BOOST_TEST_EQ(result, send_data); | |
| }(s1, s2)); | |
| ioc.run(); | |
| s1.close(); | |
| s2.close(); | |
| } | |
| void | |
| testReadString() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| std::string send_data = "Hello, this is a test message!"; | |
| co_await write(a, capy::const_buffer( | |
| send_data.data(), send_data.size())); | |
| a.close(); | |
| // Read into string until EOF | |
| std::string result; | |
| auto [ec, n] = co_await read(b, result); | |
| // EOF is expected | |
| BOOST_TEST(ec == capy::error::eof); | |
| BOOST_TEST_EQ(n, send_data.size()); | |
| BOOST_TEST_EQ(result, send_data); | |
| }(s1, s2)); | |
| ioc.run(); | |
| // s1 already closed in coroutine | |
| s2.close(); | |
| } |
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 619 - 645, In testReadString() the socket
`s1` (aliased to `a` inside the coroutine) is closed inside the coroutine and
then closed again after ioc.run() causing a double-close; fix by removing the
redundant post-run `s1.close()` (or replace it with a conditional/is_open check)
so only `s2` (or sockets that weren't closed in the coroutine) are closed
outside the coroutine; update references in testReadString(), and keep the
coroutine parameters `socket& a, socket& b` and the existing close call inside
the coroutine intact.
| void | ||
| testReadPartialEOF() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| // Send 50 bytes but try to read 100 | ||
| std::string send_data(50, 'Z'); | ||
| co_await write(a, capy::const_buffer( | ||
| send_data.data(), send_data.size())); | ||
| a.close(); | ||
|
|
||
| char buf[100] = {}; | ||
| auto [ec, n] = co_await read(b, capy::mutable_buffer( | ||
| buf, sizeof(buf))); | ||
| // Should get EOF after reading available data | ||
| BOOST_TEST(ec == capy::error::eof); | ||
| BOOST_TEST_EQ(n, 50u); | ||
| BOOST_TEST_EQ(std::string_view(buf, n), send_data); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
Double-close on s1.
Same pattern: a.close() at line 660, then s1.close() at line 672.
Suggested fix
ioc.run();
- s1.close();
+ // s1 already closed in coroutine
s2.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| testReadPartialEOF() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| // Send 50 bytes but try to read 100 | |
| std::string send_data(50, 'Z'); | |
| co_await write(a, capy::const_buffer( | |
| send_data.data(), send_data.size())); | |
| a.close(); | |
| char buf[100] = {}; | |
| auto [ec, n] = co_await read(b, capy::mutable_buffer( | |
| buf, sizeof(buf))); | |
| // Should get EOF after reading available data | |
| BOOST_TEST(ec == capy::error::eof); | |
| BOOST_TEST_EQ(n, 50u); | |
| BOOST_TEST_EQ(std::string_view(buf, n), send_data); | |
| }(s1, s2)); | |
| ioc.run(); | |
| s1.close(); | |
| s2.close(); | |
| } | |
| void | |
| testReadPartialEOF() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| // Send 50 bytes but try to read 100 | |
| std::string send_data(50, 'Z'); | |
| co_await write(a, capy::const_buffer( | |
| send_data.data(), send_data.size())); | |
| a.close(); | |
| char buf[100] = {}; | |
| auto [ec, n] = co_await read(b, capy::mutable_buffer( | |
| buf, sizeof(buf))); | |
| // Should get EOF after reading available data | |
| BOOST_TEST(ec == capy::error::eof); | |
| BOOST_TEST_EQ(n, 50u); | |
| BOOST_TEST_EQ(std::string_view(buf, n), send_data); | |
| }(s1, s2)); | |
| ioc.run(); | |
| // s1 already closed in coroutine | |
| s2.close(); | |
| } |
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 647 - 674, The test testReadPartialEOF
double-closes the same socket: the coroutine closes 'a' (the s1 socket) via
a.close(), and then the test calls s1.close() again after ioc.run(); remove the
redundant s1.close() (or conditionally skip closing s1) so the socket is only
closed once—update the cleanup at the end of testReadPartialEOF to only close s2
(or neither if both were closed in the coroutine).
| // Official repository: https://github.com/cppalliance/corosio | ||
| // | ||
|
|
||
| // Plan: c:\Users\Vinnie\.cursor\plans\tls_stream_tests_83c24f98.plan.md |
There was a problem hiding this comment.
Remove local development path reference.
This line contains a Windows-specific path to a local .cursor plans directory. It's a development artifact that shouldn't be committed to the repository.
Suggested fix
-// Plan: c:\Users\Vinnie\.cursor\plans\tls_stream_tests_83c24f98.plan.md
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Plan: c:\Users\Vinnie\.cursor\plans\tls_stream_tests_83c24f98.plan.md |
🤖 Prompt for AI Agents
In `@test/unit/tls/cross_ssl_stream.cpp` at line 10, Remove the local Windows
development path reference found in the comment on cross_ssl_stream.cpp (the
line containing
"c:\Users\Vinnie\.cursor\plans\tls_stream_tests_83c24f98.plan.md"); delete that
hard-coded path or replace it with a repository-relative or generic reference
(e.g., remove the .cursor path or replace with a neutral
"plans/tls_stream_tests.plan" or a short TODO) so no user-specific/local
filesystem paths remain in test/unit/tls/cross_ssl_stream.cpp.
| // Server private key from Boost.Beast | ||
| inline constexpr char const* server_key_pem = | ||
| "-----BEGIN PRIVATE KEY-----\n" | ||
| "MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCz0GwgnxSBhygx\n" | ||
| "BdhTHGx5LDLIJSuIDJ6nMwZFvAjdhLnB/vOTLppr5MKxqQHEpYdyDYGD1noBoz4T\n" | ||
| "iIRj5JapChMgx58NLq5QyXkHV/ONT7yi8x05P41c2F9pBEnUwUxIUG1Cb6AN0cZW\n" | ||
| "F/wSMOZ0w3DoBhnl1sdQfQiS25MTK6x4tATmWm9SJc2lsjWptbyIN6hFXLYPXTwn\n" | ||
| "YzCLvv1EK6Ft7tMPc/FcJpd/wYHgl8shDmY7rV+AiGTxUU35V0AzpJlmvct5aJV/\n" | ||
| "5vSRRLwT9qLZSddE9zy/0rovC5GML6S7BUC4lIzJ8yxzOzSStBPxvdrOobSSNlRZ\n" | ||
| "IlE7gnyNAgMBAAECggEAY0RorQmldGx9D7M+XYOPjsWLs1px0cXFwGA20kCgVEp1\n" | ||
| "kleBeHt93JqJsTKwOzN2tswl9/ZrnIPWPUpcbBlB40ggjzQk5k4jBY50Nk2jsxuV\n" | ||
| "9A9qzrP7AoqhAYTQjZe42SMtbkPZhEeOyvCqxBAi6csLhcv4eB4+In0kQo7dfvLs\n" | ||
| "Xu/3WhSsuAWqdD9EGnhD3n+hVTtgiasRe9318/3R9DzP+IokoQGOtXm+1dsfP0mV\n" | ||
| "8XGzQHBpUtJNn0yi6SC4kGEQuKkX33zORlSnZgT5VBLofNgra0THd7x3atOx1lbr\n" | ||
| "V0QizvCdBa6j6FwhOQwW8UwgOCnUbWXl/Xn4OaofMQKBgQDdRXSMyys7qUMe4SYM\n" | ||
| "Mdawj+rjv0Hg98/xORuXKEISh2snJGKEwV7L0vCn468n+sM19z62Axz+lvOUH8Qr\n" | ||
| "hLkBNqJvtIP+b0ljRjem78K4a4qIqUlpejpRLw6a/+44L76pMJXrYg3zdBfwzfwu\n" | ||
| "b9NXdwHzWoNuj4v36teGP6xOUwKBgQDQCT52XX96NseNC6HeK5BgWYYjjxmhksHi\n" | ||
| "stjzPJKySWXZqJpHfXI8qpOd0Sd1FHB+q1s3hand9c+Rxs762OXlqA9Q4i+4qEYZ\n" | ||
| "qhyRkTsl+2BhgzxmoqGd5gsVT7KV8XqtuHWLmetNEi+7+mGSFf2iNFnonKlvT1JX\n" | ||
| "4OQZC7ntnwKBgH/ORFmmaFxXkfteFLnqd5UYK5ZMvGKTALrWP4d5q2BEc7HyJC2F\n" | ||
| "+5lDR9nRezRedS7QlppPBgpPanXeO1LfoHSA+CYJYEwwP3Vl83Mq/Y/EHgp9rXeN\n" | ||
| "L+4AfjEtLo2pljjnZVDGHETIg6OFdunjkXDtvmSvnUbZBwG11bMnSAEdAoGBAKFw\n" | ||
| "qwJb6FNFM3JnNoQctnuuvYPWxwM1yjRMqkOIHCczAlD4oFEeLoqZrNhpuP8Ij4wd\n" | ||
| "GjpqBbpzyVLNP043B6FC3C/edz4Lh+resjDczVPaUZ8aosLbLiREoxE0udfWf2dU\n" | ||
| "oBNnrMwwcs6jrRga7Kr1iVgUSwBQRAxiP2CYUv7tAoGBAKdPdekPNP/rCnHkKIkj\n" | ||
| "o13pr+LJ8t+15vVzZNHwPHUWiYXFhG8Ivx7rqLQSPGcuPhNss3bg1RJiZAUvF6fd\n" | ||
| "e6QS4EZM9dhhlO2FmPQCJMrRVDXaV+9TcJZXCbclQnzzBus9pwZZyw4Anxo0vmir\n" | ||
| "nOMOU6XI4lO9Xge/QDEN4Y2R\n" | ||
| "-----END PRIVATE KEY-----\n"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repository structure and locate the test file
find . -type f -name "*.hpp" | grep -i test | head -20Repository: cppalliance/corosio
Length of output: 178
🏁 Script executed:
# Check if the file exists and examine its context
ls -la test/unit/tls/test_utils.hpp 2>/dev/null || echo "File not found in standard location"Repository: cppalliance/corosio
Length of output: 141
🏁 Script executed:
# Search for secret scanning configuration files (gitleaks, truffleHog, etc.)
find . -type f \( -name ".gitleaksignore" -o -name ".gitleaks.toml" -o -name "gitleaks.toml" -o -name ".gitignore" -o -name ".secretsignore" -o -name ".pre-commit-config.yaml" \) | head -20Repository: cppalliance/corosio
Length of output: 92
🏁 Script executed:
# Check CI configuration files for secret scanning setup
find . -type f \( -name ".github" -o -name ".gitlab-ci.yml" -o -name ".circleci" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(\.github|\.gitlab-ci|\.circleci)" | head -20Repository: cppalliance/corosio
Length of output: 89
🏁 Script executed:
# Look for other embedded test credentials/keys in the repo
rg -i "(begin (private|public|rsa) key|begin certificate|-----BEGIN)" --type cpp --type hpp --type txt | head -30Repository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
# Check if there's documentation about test fixtures or test data handling
find . -type f -name "README*" -o -name "TESTING*" -o -name "*.md" | xargs grep -l "test.*key\|fixture\|credential" 2>/dev/null | head -10Repository: cppalliance/corosio
Length of output: 94
🏁 Script executed:
# Let's examine the CI configuration
cat .github/workflows/ci.ymlRepository: cppalliance/corosio
Length of output: 21575
🏁 Script executed:
# Check .gitignore files
cat .gitignore
cat doc/.gitignoreRepository: cppalliance/corosio
Length of output: 175
🏁 Script executed:
# Read the test file to verify the embedded key and understand its purpose
head -100 test/unit/tls/test_utils.hppRepository: cppalliance/corosio
Length of output: 5259
🏁 Script executed:
# Search for other embedded credentials/keys using a broader approach
grep -r "BEGIN PRIVATE KEY\|BEGIN CERTIFICATE\|BEGIN RSA KEY" . --include="*.hpp" --include="*.cpp" --include="*.h" | head -20Repository: cppalliance/corosio
Length of output: 265
🏁 Script executed:
# Check if there are any comments in the test file about the key being a test fixture
grep -B 5 -A 5 "BEGIN PRIVATE KEY" test/unit/tls/test_utils.hpp | head -30Repository: cppalliance/corosio
Length of output: 671
Add .gitleaksignore allowlist entry for embedded test credentials. This file intentionally embeds test certificates and keys from Boost.Beast (documented in-code), but there is no secret scanning configuration (gitleaks, etc.) currently enabled in CI. While not an immediate risk, add a .gitleaksignore entry pointing to test/unit/tls/test_utils.hpp to prevent false positives if secret scanning tools are enabled in the future.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 67-94: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 Prompt for AI Agents
In `@test/unit/tls/test_utils.hpp` around lines 65 - 94, Add a .gitleaksignore
allowlist entry to prevent false positives for the embedded Boost.Beast test
credentials: identify the constant server_key_pem in test_utils.hpp and
whitelist that file (or a pattern matching this test file) in .gitleaksignore
with a brief justification mentioning the key is an intentionally embedded test
certificate from Boost.Beast.
| /** Test bidirectional data transfer on connected streams. */ | ||
| inline capy::task<> | ||
| test_stream( io_stream& a, io_stream& b ) | ||
| { | ||
| char buf[32] = {}; | ||
|
|
||
| // Write from a, read from b | ||
| auto [ec1, n1] = co_await a.write_some( | ||
| capy::const_buffer( "hello", 5 ) ); | ||
| BOOST_TEST( !ec1 ); | ||
| BOOST_TEST_EQ( n1, 5u ); | ||
|
|
||
| auto [ec2, n2] = co_await b.read_some( | ||
| capy::mutable_buffer( buf, sizeof( buf ) ) ); | ||
| BOOST_TEST( !ec2 ); | ||
| BOOST_TEST_EQ( n2, 5u ); | ||
| BOOST_TEST_EQ( std::string_view( buf, n2 ), "hello" ); | ||
|
|
||
| // Write from b, read from a | ||
| auto [ec3, n3] = co_await b.write_some( | ||
| capy::const_buffer( "world", 5 ) ); | ||
| BOOST_TEST( !ec3 ); | ||
| BOOST_TEST_EQ( n3, 5u ); | ||
|
|
||
| auto [ec4, n4] = co_await a.read_some( | ||
| capy::mutable_buffer( buf, sizeof( buf ) ) ); | ||
| BOOST_TEST( !ec4 ); | ||
| BOOST_TEST_EQ( n4, 5u ); | ||
| BOOST_TEST_EQ( std::string_view( buf, n4 ), "world" ); | ||
| } |
There was a problem hiding this comment.
read_some can return partial data; test may be flaky.
Line 222 and Line 234 assume a full 5‑byte read. read_some permits shorter reads, so this can sporadically fail. Consider looping until the expected length is collected.
🐛 Suggested fix
- auto [ec2, n2] = co_await b.read_some(
- capy::mutable_buffer( buf, sizeof( buf ) ) );
- BOOST_TEST( !ec2 );
- BOOST_TEST_EQ( n2, 5u );
- BOOST_TEST_EQ( std::string_view( buf, n2 ), "hello" );
+ std::size_t n2 = 0;
+ while( n2 < 5 )
+ {
+ auto [ec2, n] = co_await b.read_some(
+ capy::mutable_buffer( buf + n2, 5 - n2 ) );
+ BOOST_TEST( !ec2 );
+ n2 += n;
+ }
+ BOOST_TEST_EQ( n2, 5u );
+ BOOST_TEST_EQ( std::string_view( buf, n2 ), "hello" );
@@
- auto [ec4, n4] = co_await a.read_some(
- capy::mutable_buffer( buf, sizeof( buf ) ) );
- BOOST_TEST( !ec4 );
- BOOST_TEST_EQ( n4, 5u );
- BOOST_TEST_EQ( std::string_view( buf, n4 ), "world" );
+ std::size_t n4 = 0;
+ while( n4 < 5 )
+ {
+ auto [ec4, n] = co_await a.read_some(
+ capy::mutable_buffer( buf + n4, 5 - n4 ) );
+ BOOST_TEST( !ec4 );
+ n4 += n;
+ }
+ BOOST_TEST_EQ( n4, 5u );
+ BOOST_TEST_EQ( std::string_view( buf, n4 ), "world" );🤖 Prompt for AI Agents
In `@test/unit/tls/test_utils.hpp` around lines 210 - 239, The test_stream helper
assumes read_some returns the full 5 bytes which can be flaky; change the two
reads that call b.read_some and a.read_some to accumulate into buf in a loop
until the expected number of bytes (5) is received, checking the error (ec) on
each iteration and advancing the buffer pointer/count accordingly; keep the
existing write_some calls and assertions for their returned n values, but
replace the single-read assertions for ec and n with assertions after the loop
that the total bytes read equals 5 and that the concatenated data equals the
expected string ("hello" and "world"); this touches the test_stream function and
the calls to io_stream::read_some.
vinniefalco
force-pushed
the
develop
branch
2 times, most recently
from
e14d250 to
8b69a27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/corosio/src/detail/iocp/sockets.cpp (1)
193-215: Avoid danglingimpl_outwhenimpl_owner_outis null (possible UAF).If
impl_outis provided butimpl_owner_outis null,peer_impl.reset()destroys the impl while the raw pointer is still returned. Also, on failure,impl_owner_outis left untouched. Consider enforcing ownership output or returning an error, and clearimpl_owner_outon failure.🔧 Suggested safeguard
if (success && accepted_socket != INVALID_SOCKET && peer_impl) { + if (impl_out && !impl_owner_out) + { + if (ec_out) + *ec_out = make_error_code(system::errc::invalid_argument); + if (impl_out) + *impl_out = nullptr; + ::closesocket(accepted_socket); + accepted_socket = INVALID_SOCKET; + peer_impl.reset(); + d.dispatch(h).resume(); + return; + } // Pass impl and ownership to awaitable for assignment to peer socket if (impl_out) *impl_out = peer_impl.get(); if (impl_owner_out) *impl_owner_out = std::move(peer_impl); else peer_impl.reset(); } else { // Cleanup on failure if (accepted_socket != INVALID_SOCKET) { ::closesocket(accepted_socket); accepted_socket = INVALID_SOCKET; } // Release the peer_impl shared_ptr peer_impl.reset(); if (impl_out) *impl_out = nullptr; + if (impl_owner_out) + *impl_owner_out = nullptr; }
🤖 Fix all issues with AI agents
In `@test/unit/socket.cpp`:
- Around line 434-468: In testWriteAfterPeerClose(), the socket s2 (aliased as b
inside the coroutine) is closed twice: once inside the coroutine (b.close()) and
again after ioc.run() (s2.close()); remove the redundant second close or guard
it with an is_open() check. Edit the testWriteAfterPeerClose function to either
delete the trailing s2.close() call or replace it with if (s2.is_open())
s2.close(), keeping s1.close() as-is and leaving the coroutine logic (b.close(),
write loop) unchanged.
- Around line 520-564: The testCloseWhileReading test double-closes socket s2
(aliased as b): b.close() is called inside the coroutine and then s2.close() is
invoked again after ioc.run(); remove the redundant second close (or guard it
with a check) so that s2 is not closed twice — update the teardown at the end of
testCloseWhileReading to avoid calling s2.close() if the coroutine already
closed it (identify the coroutine lambda nested_coro, the local socket alias b,
and the final s2.close() call to adjust).
- Around line 402-432: In testReadAfterPeerClose: remove the redundant close of
s1 after ioc.run() because socket a (aliased to s1) is already closed inside the
coroutine via a.close(); update or remove the trailing s1.close() call (leave
s2.close() if needed) so the socket isn't closed twice and do so by editing the
testReadAfterPeerClose function to eliminate the duplicate s1.close()
invocation.
♻️ Duplicate comments (6)
include/boost/corosio/read.hpp (1)
79-84: Sync Doxygen@paramname withbs.Doc still says
buffers, but the signature usesbs. Please update to avoid drift.📚 Suggested doc fix
- `@param` buffers The buffer sequence to read data into. + `@param` bs The buffer sequence to read data into.test/unit/socket.cpp (2)
625-651: Double-close ons1(duplicate).This issue was already flagged in a previous review. Socket
a(aliased tos1) is closed inside the coroutine at line 637, thens1.close()is called again at line 649.
653-680: Double-close ons1(duplicate).This issue was already flagged in a previous review. Socket
a(aliased tos1) is closed inside the coroutine at line 666, thens1.close()is called again at line 678.test/unit/tls/test_utils.hpp (3)
65-94: Test credentials flagged by Gitleaks - already addressed in previous review.The embedded private key is intentionally included for TLS testing (sourced from Boost.Beast). A previous review already recommended adding a
.gitleaksignoreentry to prevent false positives if secret scanning is enabled.
96-114: Placeholder certificate content - already addressed in previous review.The
wrong_ca_cert_pemcontains invalid placeholder data (repeatingZcharacters in public key,Dummyprefix in signature). A previous review already flagged this as needing replacement with a real self-signed certificate to ensure the "wrong CA" scenario tests the intended verification failure rather than failing during context setup.
210-239: Partial read handling - already addressed in previous review.A previous review noted that
read_somemay return fewer than 5 bytes, making the assertions on lines 225 and 237 potentially flaky. The suggested fix was to loop until the expected length is collected.
🧹 Nitpick comments (2)
test/unit/tls/test_utils.hpp (2)
188-202: Adddefaultcase to suppress compiler warnings.The switch covers all
context_modevalues, but many compilers emit warnings about missingdefaultcases. The fallbackreturnafter the switch (line 201) handles this at runtime, but adding an explicitdefaultis cleaner.💡 Suggested fix
switch( mode ) { case context_mode::anon: return { make_anon_context(), make_anon_context() }; case context_mode::shared_cert: { auto ctx = make_server_context(); ctx.add_certificate_authority( ca_cert_pem ); return { ctx, ctx }; } case context_mode::separate_cert: return { make_client_context(), make_server_context() }; + default: + break; } return { make_anon_context(), make_anon_context() };
415-441: Potential double-close of sockets and unused captures.Two concerns:
Double-close: If the timer expires, sockets are closed in the callback (lines 426-427). Then lines 439-440 close them unconditionally again. While this is typically safe, it's redundant and could cause issues depending on the socket implementation.
Unused captures:
client_doneandserver_doneare captured but only(void)-cast (lines 418-419). They serve no purpose in the callback and can be removed.♻️ Suggested fix
capy::run_async( ioc.get_executor() )( - [&timeout, &s1, &s2, &client_done, &server_done]() -> capy::task<> + [&timeout, &s1, &s2]() -> capy::task<> { - (void)client_done; - (void)server_done; auto [ec] = co_await timeout.wait(); if( !ec ) { // Timer expired - cancel pending operations then close sockets s1.cancel(); s2.cancel(); s1.close(); s2.close(); } }() ); ioc.run(); // Cancel timer if handshakes completed before timeout timeout.cancel(); // At least one side should have failed BOOST_TEST( client_failed || server_failed ); - s1.close(); - s2.close(); + // Note: sockets may already be closed by timer callback }
| void | ||
| testReadAfterPeerClose() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| // Write data then close | ||
| co_await a.write_some(capy::const_buffer("final", 5)); | ||
| a.close(); | ||
|
|
||
| // Read the data | ||
| char buf[32] = {}; | ||
| auto [ec1, n1] = co_await b.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| BOOST_TEST(!ec1); | ||
| BOOST_TEST_EQ(std::string_view(buf, n1), "final"); | ||
|
|
||
| // Next read should get EOF (0 bytes or error) | ||
| auto [ec2, n2] = co_await b.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| // EOF indicated by error or zero bytes | ||
| BOOST_TEST(ec2 || n2 == 0); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
Double-close on s1.
Socket a (aliased to s1) is closed inside the coroutine at line 413, then s1.close() is called again at line 430 after ioc.run().
Suggested fix
ioc.run();
- s1.close();
+ // s1 already closed in coroutine
s2.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| testReadAfterPeerClose() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| // Write data then close | |
| co_await a.write_some(capy::const_buffer("final", 5)); | |
| a.close(); | |
| // Read the data | |
| char buf[32] = {}; | |
| auto [ec1, n1] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| BOOST_TEST(!ec1); | |
| BOOST_TEST_EQ(std::string_view(buf, n1), "final"); | |
| // Next read should get EOF (0 bytes or error) | |
| auto [ec2, n2] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| // EOF indicated by error or zero bytes | |
| BOOST_TEST(ec2 || n2 == 0); | |
| }(s1, s2)); | |
| ioc.run(); | |
| s1.close(); | |
| s2.close(); | |
| } | |
| void | |
| testReadAfterPeerClose() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [](socket& a, socket& b) -> capy::task<> | |
| { | |
| // Write data then close | |
| co_await a.write_some(capy::const_buffer("final", 5)); | |
| a.close(); | |
| // Read the data | |
| char buf[32] = {}; | |
| auto [ec1, n1] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| BOOST_TEST(!ec1); | |
| BOOST_TEST_EQ(std::string_view(buf, n1), "final"); | |
| // Next read should get EOF (0 bytes or error) | |
| auto [ec2, n2] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| // EOF indicated by error or zero bytes | |
| BOOST_TEST(ec2 || n2 == 0); | |
| }(s1, s2)); | |
| ioc.run(); | |
| // s1 already closed in coroutine | |
| s2.close(); | |
| } |
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 402 - 432, In testReadAfterPeerClose:
remove the redundant close of s1 after ioc.run() because socket a (aliased to
s1) is already closed inside the coroutine via a.close(); update or remove the
trailing s1.close() call (leave s2.close() if needed) so the socket isn't closed
twice and do so by editing the testReadAfterPeerClose function to eliminate the
duplicate s1.close() invocation.
| void | ||
| testWriteAfterPeerClose() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| // Close the receiving end | ||
| b.close(); | ||
|
|
||
| // Give OS time to process the close | ||
| timer t(a.context()); | ||
| t.expires_after(std::chrono::milliseconds(50)); | ||
| co_await t.wait(); | ||
|
|
||
| // Writing to closed peer should eventually fail | ||
| system::error_code last_ec; | ||
| for (int i = 0; i < 10; ++i) | ||
| { | ||
| auto [ec, n] = co_await a.write_some( | ||
| capy::const_buffer("data", 4)); | ||
| last_ec = ec; | ||
| if (ec) | ||
| break; | ||
| } | ||
| // Should get an error (broken pipe or similar) | ||
| BOOST_TEST(last_ec); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
Double-close on s2.
Socket b (aliased to s2) is closed inside the coroutine at line 444, then s2.close() is called again at line 467 after ioc.run().
Suggested fix
ioc.run();
s1.close();
- s2.close();
+ // s2 already closed in coroutine🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 434 - 468, In testWriteAfterPeerClose(),
the socket s2 (aliased as b inside the coroutine) is closed twice: once inside
the coroutine (b.close()) and again after ioc.run() (s2.close()); remove the
redundant second close or guard it with an is_open() check. Edit the
testWriteAfterPeerClose function to either delete the trailing s2.close() call
or replace it with if (s2.is_open()) s2.close(), keeping s1.close() as-is and
leaving the coroutine logic (b.close(), write loop) unchanged.
| void | ||
| testCloseWhileReading() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [&](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| timer t(a.context()); | ||
| t.expires_after(std::chrono::milliseconds(50)); | ||
|
|
||
| bool read_done = false; | ||
| system::error_code read_ec; | ||
|
|
||
| // Store lambda in variable to ensure it outlives the coroutine. | ||
| // Lambda coroutines capture 'this' by reference, so the lambda | ||
| // must remain alive while the coroutine is suspended. | ||
| auto nested_coro = [&b, &read_done, &read_ec]() -> capy::task<> | ||
| { | ||
| char buf[32]; | ||
| auto [ec, n] = co_await b.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| read_ec = ec; | ||
| read_done = true; | ||
| }; | ||
| capy::run_async(ioc.get_executor())(nested_coro()); | ||
|
|
||
| // Wait then close the socket | ||
| co_await t.wait(); | ||
| b.close(); | ||
|
|
||
| timer t2(a.context()); | ||
| t2.expires_after(std::chrono::milliseconds(50)); | ||
| co_await t2.wait(); | ||
|
|
||
| BOOST_TEST(read_done); | ||
| // Close should cancel pending operations | ||
| BOOST_TEST(read_ec == capy::cond::canceled); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
Double-close on s2.
Socket b (aliased to s2) is closed inside the coroutine at line 550, then s2.close() is called again at line 563 after ioc.run().
Suggested fix
ioc.run();
s1.close();
- s2.close();
+ // s2 already closed in coroutine📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| testCloseWhileReading() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [&](socket& a, socket& b) -> capy::task<> | |
| { | |
| timer t(a.context()); | |
| t.expires_after(std::chrono::milliseconds(50)); | |
| bool read_done = false; | |
| system::error_code read_ec; | |
| // Store lambda in variable to ensure it outlives the coroutine. | |
| // Lambda coroutines capture 'this' by reference, so the lambda | |
| // must remain alive while the coroutine is suspended. | |
| auto nested_coro = [&b, &read_done, &read_ec]() -> capy::task<> | |
| { | |
| char buf[32]; | |
| auto [ec, n] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| read_ec = ec; | |
| read_done = true; | |
| }; | |
| capy::run_async(ioc.get_executor())(nested_coro()); | |
| // Wait then close the socket | |
| co_await t.wait(); | |
| b.close(); | |
| timer t2(a.context()); | |
| t2.expires_after(std::chrono::milliseconds(50)); | |
| co_await t2.wait(); | |
| BOOST_TEST(read_done); | |
| // Close should cancel pending operations | |
| BOOST_TEST(read_ec == capy::cond::canceled); | |
| }(s1, s2)); | |
| ioc.run(); | |
| s1.close(); | |
| s2.close(); | |
| } | |
| void | |
| testCloseWhileReading() | |
| { | |
| io_context ioc; | |
| auto [s1, s2] = test::make_socket_pair(ioc); | |
| capy::run_async(ioc.get_executor())( | |
| [&](socket& a, socket& b) -> capy::task<> | |
| { | |
| timer t(a.context()); | |
| t.expires_after(std::chrono::milliseconds(50)); | |
| bool read_done = false; | |
| system::error_code read_ec; | |
| // Store lambda in variable to ensure it outlives the coroutine. | |
| // Lambda coroutines capture 'this' by reference, so the lambda | |
| // must remain alive while the coroutine is suspended. | |
| auto nested_coro = [&b, &read_done, &read_ec]() -> capy::task<> | |
| { | |
| char buf[32]; | |
| auto [ec, n] = co_await b.read_some( | |
| capy::mutable_buffer(buf, sizeof(buf))); | |
| read_ec = ec; | |
| read_done = true; | |
| }; | |
| capy::run_async(ioc.get_executor())(nested_coro()); | |
| // Wait then close the socket | |
| co_await t.wait(); | |
| b.close(); | |
| timer t2(a.context()); | |
| t2.expires_after(std::chrono::milliseconds(50)); | |
| co_await t2.wait(); | |
| BOOST_TEST(read_done); | |
| // Close should cancel pending operations | |
| BOOST_TEST(read_ec == capy::cond::canceled); | |
| }(s1, s2)); | |
| ioc.run(); | |
| s1.close(); | |
| // s2 already closed in coroutine | |
| } |
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 520 - 564, The testCloseWhileReading test
double-closes socket s2 (aliased as b): b.close() is called inside the coroutine
and then s2.close() is invoked again after ioc.run(); remove the redundant
second close (or guard it with a check) so that s2 is not closed twice — update
the teardown at the end of testCloseWhileReading to avoid calling s2.close() if
the coroutine already closed it (identify the coroutine lambda nested_coro, the
local socket alias b, and the final s2.close() call to adjust).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/acceptor.hpp (1)
272-290: epoll_acceptor_impl::accept signature is out of sync with the interface.The abstract base class
acceptor_impl::acceptnow requires 6 parameters including the newstd::shared_ptr<void>*parameter, but the epoll implementation insrc/corosio/src/detail/epoll/sockets.hppline 120-125 still has only 5 parameters. The Windows implementation (win_acceptor_impl) is correctly updated with all 6 parameters. Update the epoll signature to match:void accept( std::coroutine_handle<>, capy::any_executor_ref, std::stop_token, system::error_code*, io_object::io_object_impl**, std::shared_ptr<void>*) override;Then update the implementation in the corresponding .cpp file to handle the new parameter.
🤖 Fix all issues with AI agents
In `@test/unit/socket.cpp`:
- Around line 349-393: The loops in testLargeBuffer calling write_some and
read_some can spin forever if an I/O error occurs because BOOST_TEST(!ec)
doesn't abort and total_sent/total_recv won't advance; update both loops (the
write loop around a.write_some and the read loop around b.read_some) to check
for error or zero bytes (if ec || n == 0) immediately after the co_await, and on
that condition fail the test and exit the coroutine (e.g., call
BOOST_TEST(false) or equivalent and co_return/return) so the test stops instead
of looping indefinitely.
♻️ Duplicate comments (5)
test/unit/socket.cpp (5)
402-432: Avoid double-close ons1.
a.close()happens in the coroutine, thens1.close()runs afterioc.run().
434-468: Avoid double-close ons2.
b.close()happens in the coroutine, thens2.close()runs afterioc.run().
520-564: Avoid double-close ons2.
b.close()happens in the coroutine, thens2.close()runs afterioc.run().
625-651: Avoid double-close ons1.
a.close()happens in the coroutine, thens1.close()runs afterioc.run().
653-680: Avoid double-close ons1.
a.close()happens in the coroutine, thens1.close()runs afterioc.run().
🧹 Nitpick comments (2)
src/corosio/src/detail/iocp/sockets.hpp (1)
55-55: Consider documenting why operations override operator().The operations
connect_op,read_op, andwrite_opnow overrideoperator()(declared on lines 55, 71, 87). From the .cpp file, these overrides call the base class then resetimpl_ptr. A brief doc comment would clarify this pattern for maintainers.Also applies to: 71-71, 87-87
src/corosio/src/detail/iocp/sockets.cpp (1)
495-502: Missingempty_buffer = truefor write_some empty buffer case.For consistency with
read_some, consider settingop.empty_buffer = truehere. While it doesn't affect EOF detection (writes don't trigger EOF), maintaining symmetry improves code clarity and future-proofs against potential uses of the flag.Suggested change
// Handle empty buffer: complete immediately with 0 bytes if (op.wsabuf_count == 0) { op.bytes_transferred = 0; op.error = 0; + op.empty_buffer = true; svc_.post(&op); return; }
| void | ||
| testLargeBuffer() | ||
| { | ||
| io_context ioc; | ||
| auto [s1, s2] = test::make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| // 64KB data - larger than typical TCP segment | ||
| constexpr std::size_t size = 64 * 1024; | ||
| std::vector<char> send_data(size); | ||
| for (std::size_t i = 0; i < size; ++i) | ||
| send_data[i] = static_cast<char>(i & 0xFF); | ||
|
|
||
| std::vector<char> recv_data(size); | ||
| std::size_t total_sent = 0; | ||
| std::size_t total_recv = 0; | ||
|
|
||
| // Send all data (may take multiple write_some calls) | ||
| while (total_sent < size) | ||
| { | ||
| auto [ec, n] = co_await a.write_some( | ||
| capy::const_buffer( | ||
| send_data.data() + total_sent, | ||
| size - total_sent)); | ||
| BOOST_TEST(!ec); | ||
| total_sent += n; | ||
| } | ||
|
|
||
| // Receive all data (may take multiple read_some calls) | ||
| while (total_recv < size) | ||
| { | ||
| auto [ec, n] = co_await b.read_some( | ||
| capy::mutable_buffer( | ||
| recv_data.data() + total_recv, | ||
| size - total_recv)); | ||
| BOOST_TEST(!ec); | ||
| total_recv += n; | ||
| } | ||
|
|
||
| BOOST_TEST_EQ(total_sent, size); | ||
| BOOST_TEST_EQ(total_recv, size); | ||
| BOOST_TEST(send_data == recv_data); | ||
| }(s1, s2)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for read_some and write_some definitions in headers
find . -type f -name "*.hpp" -o -name "*.h" | head -20Repository: cppalliance/corosio
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Search for read_some/write_some contract details
rg -n "read_some|write_some" --type-list | head -10Repository: cppalliance/corosio
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Find socket implementations and their documentation
fd -e hpp -e h | xargs grep -l "read_some\|write_some" | head -10Repository: cppalliance/corosio
Length of output: 434
🏁 Script executed:
#!/bin/bash
# Look for capy socket implementation
find . -path "*/capy*" -name "*.hpp" -o -path "*/capy*" -name "*.h" | head -20Repository: cppalliance/corosio
Length of output: 45
🌐 Web query:
BOOST_TEST framework behavior continue on failure abort
💡 Result:
Short answer:
- BOOST_CHECK, BOOST_CHECK_EQUAL, BOOST_CHECK_MESSAGE, BOOST_ERROR — report a failure and continue executing the current test case (non‑fatal). [1]
- BOOST_REQUIRE, BOOST_FAIL (and throwing an exception) — abort the current test case immediately (fatal for that test case). [1]
- Fatal system errors (signals / access violations) are handled separately by the framework (caught by default); you can disable catching to get a core dump or to let the process abort. Use --catch_system_errors=no to let such failures abort the process. [2][3]
Sources:
[1] Boost.Test tutorial / examples (behavior of BOOST_CHECK vs BOOST_REQUIRE).
[2] Boost.Test command-line usage recommendations (catch_system_errors).
[3] Boost.Test reference: catch_system_errors option.
🌐 Web query:
read_some write_some contract zero bytes return asio
💡 Result:
Short answer: read_some/write_some are allowed to return 0 only in these cases:
- You supplied an empty buffer sequence (total buffer size == 0) — then 0 bytes are read/written. [1][2]
- An error occurred (including EOF for reads). With the throwing overloads read_some throws (and read signals EOF via boost::asio::error::eof); with the ec overloads the error is reported in ec and write_some's ec-overload returns 0 on error. [1][3]
Otherwise the documented contract is that the call blocks (or completes) only after one or more bytes are transferred or an error occurs — so a successful non-error call should return >0. [1][3][4]
Sources:
- Boost.Asio basic_stream_socket::read_some (remarks/return/exceptions). [1]
- Boost.Asio basic_stream_socket::write_some (return-value notes; ec overload returns 0 on error). [3]
- Common practical cause: empty buffer -> read_some/read_some returns 0. (explained in StackOverflow answer). [2]
🏁 Script executed:
#!/bin/bash
# Read socket.hpp to understand read_some/write_some implementation
wc -l include/boost/corosio/socket.hppRepository: cppalliance/corosio
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for read_some and write_some in socket.hpp
rg -n "read_some|write_some" include/boost/corosio/socket.hpp -A 5Repository: cppalliance/corosio
Length of output: 393
🏁 Script executed:
#!/bin/bash
# Look at io_result to understand the return type
cat include/boost/corosio/io_result.hppRepository: cppalliance/corosio
Length of output: 5124
🏁 Script executed:
#!/bin/bash
# Read socket.hpp to find method implementations
head -100 include/boost/corosio/socket.hppRepository: cppalliance/corosio
Length of output: 2915
🏁 Script executed:
#!/bin/bash
# Search for read_some method in socket.hpp
rg -n "read_some" include/boost/corosio/socket.hpp -B 2 -A 10Repository: cppalliance/corosio
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Search for write_some method in socket.hpp
rg -n "write_some" include/boost/corosio/socket.hpp -B 2 -A 10Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for read_some and write_some across all headers
rg -n "read_some|write_some" include/ -A 5Repository: cppalliance/corosio
Length of output: 13377
🏁 Script executed:
#!/bin/bash
# Check the io_stream base class
cat include/boost/corosio/io_stream.hppRepository: cppalliance/corosio
Length of output: 7903
Add guards to prevent infinite loops on write/read errors.
The test loops calling write_some/read_some without checking for errors that stop progress. Per the documented contract, these functions return n=0 when an error occurs (ec != 0). Since BOOST_TEST(!ec) doesn't abort the test, a failed I/O operation leaves total_sent/total_recv unchanged, causing the loop to spin indefinitely.
Add early exits on ec || n == 0:
Suggested guards
while (total_sent < size)
{
auto [ec, n] = co_await a.write_some(
capy::const_buffer(
send_data.data() + total_sent,
size - total_sent));
BOOST_TEST(!ec);
+ if (ec || n == 0)
+ co_return;
total_sent += n;
}
@@
while (total_recv < size)
{
auto [ec, n] = co_await b.read_some(
capy::mutable_buffer(
recv_data.data() + total_recv,
size - total_recv));
BOOST_TEST(!ec);
+ if (ec || n == 0)
+ co_return;
total_recv += n;
}🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 349 - 393, The loops in testLargeBuffer
calling write_some and read_some can spin forever if an I/O error occurs because
BOOST_TEST(!ec) doesn't abort and total_sent/total_recv won't advance; update
both loops (the write loop around a.write_some and the read loop around
b.read_some) to check for error or zero bytes (if ec || n == 0) immediately
after the co_await, and on that condition fail the test and exit the coroutine
(e.g., call BOOST_TEST(false) or equivalent and co_return/return) so the test
stops instead of looping indefinitely.
2afcddc to
e7586ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/corosio/src/detail/iocp/sockets.cpp (1)
184-226: Guardpeer_wrapperbefore release to prevent null deref.
On error paths wherepeer_wrapperwas never assigned, the failure branch dereferences a null pointer and will crash.🐛 Proposed fix
- // Release the peer wrapper on failure - peer_wrapper->release(); - peer_wrapper = nullptr; + // Release the peer wrapper on failure (if it was created) + if (peer_wrapper) + { + peer_wrapper->release(); + peer_wrapper = nullptr; + }Also applies to: 231-241
🤖 Fix all issues with AI agents
In `@test/unit/socket.cpp`:
- Around line 110-121: The test assumes read_some returns the full 5 bytes but
read_some may return a partial read; update the coroutine where s1/s2 call
write_some/read_some to accumulate bytes until the expected count: replace the
single co_await b.read_some(...) usage with a loop that repeatedly co_await
b.read_some(capy::mutable_buffer(buf + offset, remaining)) and advances offset
until 5 bytes are read, or alternatively call corosio::read(b,
capy::mutable_buffer(buf, 5)) to get exactly 5 bytes; apply the same pattern to
the other failing places that use read_some (lines referenced in the review).
- Around line 451-462: The test assumes writes fail within 10 attempts; instead
make the failure deterministic by configuring the peer socket to force an
immediate close (set SO_LINGER l_onoff=1, l_linger=0) before closing it so
writes on a.write_some(...) reliably return an error; locate the peer socket
close code in this test and set the linger option there, or if changing socket
options is undesirable, increase the loop iterations (e.g., to 1000) and/or
relax the assertion by accepting either a successful write sequence followed by
a non-zero last_ec or a non-zero last_ec within the loop (replace the strict
BOOST_TEST(last_ec) with a check that allows both outcomes).
♻️ Duplicate comments (2)
test/unit/socket.cpp (2)
368-387: Duplicate: addec || n == 0guards in the large-buffer loops.
This matches a prior comment; the loop can spin forever if an error occurs andnstays 0.
413-431: Duplicate: remove redundant closes after the coroutine already closed the socket.
These tests closea/binside the coroutine, then close the same socket again afterioc.run().Also applies to: 444-467, 548-563, 634-650, 662-679
🧹 Nitpick comments (2)
test/unit/socket.cpp (1)
14-25: Add explicit STL headers for new standard types.This file now uses
std::string,std::string_view,std::vector,std::array,std::chrono, andstd::strlen. Relying on transitive includes is brittle; please include the direct headers.Suggested include additions
`#include` <boost/capy/cond.hpp> `#include` <boost/capy/error.hpp> `#include` <boost/capy/ex/run_async.hpp> `#include` <boost/capy/task.hpp> + +#include <array> +#include <chrono> +#include <cstring> +#include <string> +#include <string_view> +#include <vector>test/unit/acceptor.cpp (1)
90-137: Avoid fixed 50 ms delays to reduce flaky cancellation tests.
Timer-based waits can be brittle under CI load. Consider awaiting an explicit signal (e.g., a condition/future) or using a bounded wait loop with a more generous timeout.Also applies to: 143-188
| auto [ec1, n1] = co_await a.write_some( | ||
| capy::const_buffer("hello", 5)); | ||
| BOOST_TEST(!ec1); | ||
| BOOST_TEST_EQ(n1, 5u); | ||
|
|
||
| char buf[32] = {}; | ||
| auto [ec2, n2] = co_await b.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| BOOST_TEST(!ec2); | ||
| BOOST_TEST_EQ(n2, 5u); | ||
| BOOST_TEST_EQ(std::string_view(buf, n2), "hello"); | ||
| }(s1, s2)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the test file exists and examine the specific lines mentioned
wc -l test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 87
🏁 Script executed:
# Look at the first mentioned section (lines 110-121)
sed -n '110,121p' test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 560
🏁 Script executed:
# Check other mentioned sections to see the pattern
sed -n '146,151p' test/unit/socket.cpp
sed -n '174,180p' test/unit/socket.cpp
sed -n '199,218p' test/unit/socket.cpp
sed -n '237,272p' test/unit/socket.cpp
sed -n '415,420p' test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 3521
🏁 Script executed:
# Search for any usage of corosio::read or similar read functions
rg 'corosio::|read_some|read\(' test/unit/socket.cpp -A 2 -B 2Repository: cppalliance/corosio
Length of output: 5633
🏁 Script executed:
# Check the header files or interface definitions to understand read_some behavior
find . -name "*.hpp" -o -name "*.h" | head -20Repository: cppalliance/corosio
Length of output: 872
🏁 Script executed:
# Look for socket or buffer related headers
rg 'def.*read_some|void.*read_some|auto.*read_some' --type cppRepository: cppalliance/corosio
Length of output: 3460
🏁 Script executed:
# Find the read() function definition/declaration
rg 'auto read\(' include/boost/corosio/ -A 5Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Also check for documentation or comments about read_some vs read
rg 'read_some|read\(' include/boost/corosio/io_stream.hpp -B 2 -A 3Repository: cppalliance/corosio
Length of output: 1266
🏁 Script executed:
# Look at the read.hpp header to understand the read function
cat include/boost/corosio/read.hpp | head -100Repository: cppalliance/corosio
Length of output: 3255
🏁 Script executed:
# Look at the complete context around lines 110-121 to see the full test
sed -n '100,130p' test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 887
🏁 Script executed:
# Check one of the other flagged sections to see if they have similar patterns
sed -n '237,280p' test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 1772
🏁 Script executed:
# Verify if lines 415-420 use read() or read_some()
sed -n '410,425p' test/unit/socket.cppRepository: cppalliance/corosio
Length of output: 737
Guard against partial reads in read_some tests to avoid flakiness.
read_some() is explicitly documented to "return after reading any amount of data." The flagged tests assume full-frame reads: they write N bytes and expect read_some() to return exactly N bytes. This is not guaranteed and can fail on different platforms or under timing variations. Either loop until the expected byte count is accumulated, or use corosio::read() which does this automatically.
Example fix pattern (apply similarly to other tests)
char buf[32] = {};
auto [ec2, n2] = co_await b.read_some(
capy::mutable_buffer(buf, sizeof(buf)));
BOOST_TEST(!ec2);
BOOST_TEST_EQ(n2, 5u);
BOOST_TEST_EQ(std::string_view(buf, n2), "hello");Replace with either a loop (as originally shown) or use the corosio::read() function, which already handles this:
char buf[32] = {};
auto [ec2, n2] = co_await read(b, capy::mutable_buffer(buf, 5));
BOOST_TEST(!ec2);
BOOST_TEST_EQ(std::string_view(buf, n2), "hello");Also applies to: 146-151, 174-180, 199-218, 237-272, 415-420
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 110 - 121, The test assumes read_some
returns the full 5 bytes but read_some may return a partial read; update the
coroutine where s1/s2 call write_some/read_some to accumulate bytes until the
expected count: replace the single co_await b.read_some(...) usage with a loop
that repeatedly co_await b.read_some(capy::mutable_buffer(buf + offset,
remaining)) and advances offset until 5 bytes are read, or alternatively call
corosio::read(b, capy::mutable_buffer(buf, 5)) to get exactly 5 bytes; apply the
same pattern to the other failing places that use read_some (lines referenced in
the review).
| // Writing to closed peer should eventually fail | ||
| system::error_code last_ec; | ||
| for (int i = 0; i < 10; ++i) | ||
| { | ||
| auto [ec, n] = co_await a.write_some( | ||
| capy::const_buffer("data", 4)); | ||
| last_ec = ec; | ||
| if (ec) | ||
| break; | ||
| } | ||
| // Should get an error (broken pipe or similar) | ||
| BOOST_TEST(last_ec); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file and understand the context around lines 451-462
cat -n test/unit/socket.cpp | head -n 470 | tail -n 50Repository: cppalliance/corosio
Length of output: 1889
🏁 Script executed:
# Find the make_socket_pair implementation
rg -n "make_socket_pair" test/unit/socket.cpp -A 20 -B 2Repository: cppalliance/corosio
Length of output: 17346
🏁 Script executed:
# Find the make_socket_pair implementation in test headers
find test -name "*.hpp" -o -name "*.h" | xargs grep -l "make_socket_pair"Repository: cppalliance/corosio
Length of output: 91
🏁 Script executed:
# Look at the make_socket_pair implementation
cat test/unit/tls/test_utils.hpp | grep -A 30 "make_socket_pair"Repository: cppalliance/corosio
Length of output: 2973
🏁 Script executed:
# Search for the actual make_socket_pair function definition
rg "make_socket_pair\s*\(" test/ -A 15 | head -60Repository: cppalliance/corosio
Length of output: 3594
🏁 Script executed:
# Search for make_socket_pair function definition (with return type and signature)
rg "make_socket_pair" test/ -B 2 | grep -E "^\s*(auto|std::pair|inline|constexpr)" -A 10 | head -40Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Find all header files in test directory
find test -name "*.hpp" -o -name "*.h" | head -20Repository: cppalliance/corosio
Length of output: 91
🏁 Script executed:
# Search for the function definition of make_socket_pair
rg "^.*make_socket_pair" test/ -B 3 -A 15Repository: cppalliance/corosio
Length of output: 25181
🏁 Script executed:
# Search for the namespace and function definition
rg "namespace test" test/ -A 30 | head -80Repository: cppalliance/corosio
Length of output: 4242
🏁 Script executed:
# Read the socket_pair.cpp file to find make_socket_pair definition
wc -l test/unit/test/socket_pair.cppRepository: cppalliance/corosio
Length of output: 96
🏁 Script executed:
# Read the entire socket_pair.cpp file
cat -n test/unit/test/socket_pair.cppRepository: cppalliance/corosio
Length of output: 3213
🏁 Script executed:
# Find the socket_pair.hpp header
find test -name "socket_pair.hpp"Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search for socket_pair.hpp in include directories
find . -name "socket_pair.hpp"Repository: cppalliance/corosio
Length of output: 107
🏁 Script executed:
# Read the socket_pair.hpp header file
cat ./include/boost/corosio/test/socket_pair.hppRepository: cppalliance/corosio
Length of output: 1072
🏁 Script executed:
# Find the implementation of make_socket_pair
find . -name "*.cpp" -type f | xargs grep -l "make_socket_pair" | grep -v test/Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search for the implementation in all cpp files
rg "make_socket_pair\s*\(" -g "*.cpp" -A 30 --type cpp | head -100Repository: cppalliance/corosio
Length of output: 5771
Test may fail on platforms where TCP buffers multiple writes after peer close. The loop assumes an error occurs within 10 write attempts, but TCP acceptance of writes after close is platform-dependent. Increase loop iterations, set a socket option like SO_LINGER or TCP_NODELAY to make error behavior deterministic, or accept both outcomes (successful writes followed by eventual error).
🤖 Prompt for AI Agents
In `@test/unit/socket.cpp` around lines 451 - 462, The test assumes writes fail
within 10 attempts; instead make the failure deterministic by configuring the
peer socket to force an immediate close (set SO_LINGER l_onoff=1, l_linger=0)
before closing it so writes on a.write_some(...) reliably return an error;
locate the peer socket close code in this test and set the linger option there,
or if changing socket options is undesirable, increase the loop iterations
(e.g., to 1000) and/or relax the assertion by accepting either a successful
write sequence followed by a non-zero last_ec or a non-zero last_ec within the
loop (replace the strict BOOST_TEST(last_ec) with a check that allows both
outcomes).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/op.hpp`:
- Around line 90-100: The epoll backend is treating any 0-byte read as EOF; add
an empty_buffer flag to epoll_op and use it to suppress EOF for intentional
zero-length reads: add a member bool empty_buffer = false to epoll_op, reset it
to false in epoll_op::reset(), set empty_buffer = (iovec_count == 0) inside
epoll_op::perform_io() for read operations, and change the EOF guard in the
completion path to check !empty_buffer (i.e. else if (is_read_operation() &&
bytes_transferred == 0 && !empty_buffer)). Ensure all references are to
epoll_op, reset(), perform_io(), is_read_operation(), and bytes_transferred so
behavior matches the IOCP semantics.
In `@src/corosio/src/detail/iocp/sockets.cpp`:
- Around line 220-222: The code unconditionally dereferences peer_wrapper
causing a null pointer deref when op.peer_wrapper was set to nullptr earlier;
update the handling in sockets.cpp so you check for null before calling methods
on peer_wrapper: in the failure path where you currently call
peer_wrapper->release(); peer_wrapper = nullptr; wrap that in if (peer_wrapper)
{ peer_wrapper->release(); peer_wrapper = nullptr; } and likewise in the
completion operator (operator()) ensure the else branch only accesses or
releases op.peer_wrapper if op.peer_wrapper != nullptr to avoid dereferencing a
null pointer.
🧹 Nitpick comments (1)
src/corosio/src/detail/iocp/sockets.cpp (1)
650-669: Consider combining lock acquisitions for atomicity.The function acquires the mutex twice: once to add the internal to
socket_list_, and again to add the wrapper tosocket_wrapper_list_. While this works correctly, combining them into a single critical section would be more efficient and provide atomic visibility of both additions.♻️ Suggested consolidation
win_socket_impl& win_sockets:: create_impl() { auto internal = std::make_shared<win_socket_impl_internal>(*this); + auto* wrapper = new win_socket_impl(internal); { std::lock_guard<win_mutex> lock(mutex_); socket_list_.push_back(internal.get()); - } - - auto* wrapper = new win_socket_impl(std::move(internal)); - - { - std::lock_guard<win_mutex> lock(mutex_); socket_wrapper_list_.push_back(wrapper); } return *wrapper; }
| if (ec_out) | ||
| { | ||
| if (cancelled.load(std::memory_order_acquire)) | ||
| *ec_out = make_error_code(system::errc::operation_canceled); | ||
| else if (error != 0) | ||
| *ec_out = system::error_code(error, system::system_category()); | ||
| *ec_out = capy::error::canceled; | ||
| else if (errn != 0) | ||
| *ec_out = make_err(errn); | ||
| else if (is_read_operation() && bytes_transferred == 0) | ||
| { | ||
| // EOF: 0 bytes transferred with no error indicates end of stream | ||
| *ec_out = make_error_code(capy::error::eof); | ||
| *ec_out = capy::error::eof; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "op.hpp" | head -20Repository: cppalliance/corosio
Length of output: 100
🏁 Script executed:
wc -l src/corosio/src/detail/epoll/op.hppRepository: cppalliance/corosio
Length of output: 102
🏁 Script executed:
sed -n '80,110p' src/corosio/src/detail/epoll/op.hppRepository: cppalliance/corosio
Length of output: 883
🏁 Script executed:
# Also check the broader context of the perform_io and reset methods
sed -n '1,150p' src/corosio/src/detail/epoll/op.hppRepository: cppalliance/corosio
Length of output: 3683
🏁 Script executed:
# Search for IOCP implementation to compare EOF handling
find . -type f -name "*.hpp" -o -name "*.h" | xargs rg -l "iocp|IOCP" | head -10Repository: cppalliance/corosio
Length of output: 514
🏁 Script executed:
sed -n '1,200p' src/corosio/src/detail/iocp/overlapped_op.hppRepository: cppalliance/corosio
Length of output: 4334
🏁 Script executed:
# Search for EOF handling in IOCP
rg -A 5 -B 5 "eof|EOF" src/corosio/src/detail/iocp/overlapped_op.hppRepository: cppalliance/corosio
Length of output: 813
🏁 Script executed:
# Search for empty_buffer or zero-length read handling in IOCP
rg -A 5 -B 5 "empty|buffer|bytes_transferred == 0" src/corosio/src/detail/iocp/overlapped_op.hppRepository: cppalliance/corosio
Length of output: 1319
🏁 Script executed:
# Find read operation implementations in epoll to see if zero-length reads can happen
find . -path "*epoll*" -name "*.hpp" -o -name "*.cpp" | xargs rg -l "is_read_operation|perform_io" | head -10Repository: cppalliance/corosio
Length of output: 145
🏁 Script executed:
# Check the epoll scheduler to see if empty-buffer reads are issued
sed -n '1,100p' src/corosio/src/detail/epoll/scheduler.cppRepository: cppalliance/corosio
Length of output: 2379
🏁 Script executed:
# Search for socket read operations in epoll
rg -A 10 "perform_io" src/corosio/src/detail/epoll/scheduler.cpp | head -50Repository: cppalliance/corosio
Length of output: 313
🏁 Script executed:
# Find all files in epoll directory to understand read operation implementation
find . -path "*epoll*" -type f \( -name "*.hpp" -o -name "*.cpp" \)Repository: cppalliance/corosio
Length of output: 285
🏁 Script executed:
# Check sockets.hpp for read operation implementations
cat -n src/corosio/src/detail/epoll/sockets.hpp | head -200Repository: cppalliance/corosio
Length of output: 6816
🏁 Script executed:
# Search for readv usage and iovec handling
rg -B 5 -A 10 "readv|iovec_count" src/corosio/src/detail/epoll/sockets.hppRepository: cppalliance/corosio
Length of output: 1585
🏁 Script executed:
# Look for the complete read operation class definition
rg -B 10 -A 20 "is_read_operation\|perform_io" src/corosio/src/detail/epoll/sockets.hpp | head -100Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Get the complete read operation class definitions for epoll
rg -B 5 -A 30 "struct epoll_read_op|struct epoll_write_op" src/corosio/src/detail/epoll/sockets.hppRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check how the read_some method completes to understand the flow better
rg -B 10 -A 30 "void read_some" src/corosio/src/detail/epoll/sockets.hppRepository: cppalliance/corosio
Length of output: 1014
🏁 Script executed:
# Find where epoll_read_op is defined - search in all files
find . -path "*epoll*" -type f \( -name "*.hpp" -o -name "*.cpp" \) -exec grep -l "struct epoll_read_op\|class epoll_read_op" {} \;Repository: cppalliance/corosio
Length of output: 100
🏁 Script executed:
# Look for all operation classes across epoll files
rg "struct epoll.*_op" src/corosio/src/detail/epoll/Repository: cppalliance/corosio
Length of output: 467
🏁 Script executed:
# Get the full sockets.hpp file since it's not huge
wc -l src/corosio/src/detail/epoll/sockets.hppRepository: cppalliance/corosio
Length of output: 107
🏁 Script executed:
# Read the entire sockets.hpp file
cat src/corosio/src/detail/epoll/sockets.hppRepository: cppalliance/corosio
Length of output: 14926
Add empty_buffer flag to suppress EOF for intentional empty-buffer reads on epoll.
The epoll backend reports EOF (0 bytes transferred with no error) for any zero-byte read. IOCP already suppresses this for intentional empty-buffer reads via an empty_buffer flag. Since read_some can issue zero-length reads (handled at line 322–328 in sockets.hpp), epoll will mis-report EOF and diverge cross-platform. Track empty_buffer in epoll_op to match IOCP semantics:
- Add
bool empty_buffer = false;member to epoll_op - Reset it in
reset() - Set
empty_buffer = (iovec_count == 0)inperform_io()for read operations - Guard EOF detection:
else if (is_read_operation() && bytes_transferred == 0 && !empty_buffer)
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/epoll/op.hpp` around lines 90 - 100, The epoll backend
is treating any 0-byte read as EOF; add an empty_buffer flag to epoll_op and use
it to suppress EOF for intentional zero-length reads: add a member bool
empty_buffer = false to epoll_op, reset it to false in epoll_op::reset(), set
empty_buffer = (iovec_count == 0) inside epoll_op::perform_io() for read
operations, and change the EOF guard in the completion path to check
!empty_buffer (i.e. else if (is_read_operation() && bytes_transferred == 0 &&
!empty_buffer)). Ensure all references are to epoll_op, reset(), perform_io(),
is_read_operation(), and bytes_transferred so behavior matches the IOCP
semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/iocp/sockets.cpp`:
- Around line 619-647: Holding mutex_ while deleting wrapper objects
(socket_wrapper_list_ and acceptor_wrapper_list_) can deadlock if their
destructors call unregister_impl() which re-enters the same mutex; instead,
under mutex_ move or pop the wrapper pointers into local containers (e.g.,
std::vector or list) and release the lock, then iterate those local containers
and delete each wrapper outside the lock; update the cleanup loop that currently
deletes w from socket_wrapper_list_ and acceptor_wrapper_list_ so it only
collects pointers under mutex_ and performs deletion after unlocking to avoid
re-entrant mutex acquisition in unregister_impl().
In `@test/unit/acceptor.cpp`:
- Around line 94-137: The test uses fixed 50ms sleeps (timer t2) to wait for the
accept coroutine to finish in testCancelAccept (and similarly in the block on
lines 143-188); instead of sleeping, capture the async launch of nested_coro by
storing the capy::run_async(...) call (e.g. auto accept_task =
capy::run_async(ioc.get_executor())(nested_coro());) and then co_await
accept_task to wait deterministically for the accept to complete (optionally
wrap that await in a generous overall timeout if you must bound the test);
remove the t2 timer and the fixed sleeps and replace them with awaiting
accept_task, and apply the same change to the other similar test block.
♻️ Duplicate comments (6)
src/corosio/src/detail/epoll/op.hpp (1)
174-196: empty_buffer_read is never set, so EOF suppression won’t trigger.As written,
empty_buffer_readremainsfalse, so zero-length reads still look like EOF. Set it based oniovec_countbefore completing the read.🛠️ Proposed fix
void perform_io() noexcept override { + empty_buffer_read = (iovec_count == 0); ssize_t n = ::readv(fd, iovecs, iovec_count); if (n >= 0) complete(0, static_cast<std::size_t>(n)); else complete(errno, 0); }test/unit/socket.cpp (4)
110-120: Handle partialread_someresults to prevent flaky assertions.
read_somecan legally return fewer bytes than requested without error, so expecting full messages in a single call is brittle (especially after peer close). Prefercorosio::read()or a loop to accumulate the expected byte count.✅ Example fix pattern (apply similarly to the other read_some sites)
- auto [ec2, n2] = co_await b.read_some( - capy::mutable_buffer(buf, sizeof(buf))); + auto [ec2, n2] = co_await read( + b, capy::mutable_buffer(buf, 5));Also applies to: 146-150, 174-180, 201-218, 243-272, 416-420
368-387: Guard large-buffer loops against no‑progress to avoid hangs.If
write_some/read_somereturnsecorn == 0,BOOST_TESTwon’t abort and the loops can spin forever. Add a no‑progress guard after the await.✅ Suggested guard
while (total_sent < size) { auto [ec, n] = co_await a.write_some( capy::const_buffer( send_data.data() + total_sent, size - total_sent)); BOOST_TEST(!ec); + if (ec || n == 0) + co_return; total_sent += n; } @@ while (total_recv < size) { auto [ec, n] = co_await b.read_some( capy::mutable_buffer( recv_data.data() + total_recv, size - total_recv)); BOOST_TEST(!ec); + if (ec || n == 0) + co_return; total_recv += n; }
429-431: Avoid double-closing sockets already closed inside the coroutine.Several tests close a socket inside the coroutine and then close the same socket again after
ioc.run(). Consider removing the redundant close or guard withis_open().✅ Example adjustment (apply similarly in the other tests)
ioc.run(); - s1.close(); + if (s1.is_open()) s1.close(); s2.close();Also applies to: 465-468, 561-564, 648-651, 677-680
451-462: Make write‑after‑close behavior deterministic across platforms.The test assumes an error within 10 writes, but TCP buffering can allow many successful writes after peer close on some platforms. Consider forcing a hard close (e.g., SO_LINGER with zero timeout), increasing the iteration count, or relaxing the assertion to accept either outcome.
src/corosio/src/detail/iocp/sockets.cpp (1)
181-226: Guardpeer_wrapperbefore release to avoid null deref.Line 220-222:
peer_wrappercan be set tonullptron earlier error paths, so this unconditional release can crash.🐛 Proposed fix
- // Release the peer wrapper on failure - peer_wrapper->release(); - peer_wrapper = nullptr; + // Release the peer wrapper on failure (may be null if early error) + if (peer_wrapper) + { + peer_wrapper->release(); + peer_wrapper = nullptr; + }
🧹 Nitpick comments (1)
include/boost/corosio/acceptor.hpp (1)
30-30: Remove unused<memory>include.The
<memory>header is included at line 30 but no facilities from it (e.g.,std::unique_ptr,std::shared_ptr,std::make_unique) are used in this file. All pointers are raw pointers (impl_,peer_impl_).Defer the include until smart pointers are actually used to reduce unnecessary compilation overhead.
| std::lock_guard<win_mutex> lock(mutex_); | ||
|
|
||
| // Just close sockets and remove from list | ||
| // The shared_ptrs held by socket objects and operations will handle destruction | ||
| for (auto* impl = socket_list_.pop_front(); impl != nullptr; | ||
| impl = socket_list_.pop_front()) | ||
| { | ||
| impl->close_socket(); | ||
| delete impl; | ||
| // Note: impl may still be alive if operations hold shared_ptr | ||
| } | ||
|
|
||
| for (auto* impl = acceptor_list_.pop_front(); impl != nullptr; | ||
| impl = acceptor_list_.pop_front()) | ||
| { | ||
| impl->close_socket(); | ||
| delete impl; | ||
| } | ||
|
|
||
| // Cleanup wrappers | ||
| for (auto* w = socket_wrapper_list_.pop_front(); w != nullptr; | ||
| w = socket_wrapper_list_.pop_front()) | ||
| { | ||
| delete w; | ||
| } | ||
|
|
||
| for (auto* w = acceptor_wrapper_list_.pop_front(); w != nullptr; | ||
| w = acceptor_wrapper_list_.pop_front()) | ||
| { | ||
| delete w; | ||
| } |
There was a problem hiding this comment.
Avoid deadlock when deleting wrappers under the service mutex.
Line 637-647: Deleting wrappers while holding mutex_ can deadlock if the internal destructor calls unregister_impl() and tries to re-lock the same mutex. Consider detaching wrapper pointers under the lock, then delete after releasing the lock.
🐛 Proposed fix (delete wrappers after releasing the lock)
+#include <vector> void
win_sockets::
shutdown()
{
- std::lock_guard<win_mutex> lock(mutex_);
+ std::vector<win_socket_impl*> socket_wrappers;
+ std::vector<win_acceptor_impl*> acceptor_wrappers;
+
+ {
+ std::lock_guard<win_mutex> lock(mutex_);
// Just close sockets and remove from list
// The shared_ptrs held by socket objects and operations will handle destruction
@@
- // Cleanup wrappers
- for (auto* w = socket_wrapper_list_.pop_front(); w != nullptr;
- w = socket_wrapper_list_.pop_front())
- {
- delete w;
- }
-
- for (auto* w = acceptor_wrapper_list_.pop_front(); w != nullptr;
- w = acceptor_wrapper_list_.pop_front())
- {
- delete w;
- }
+ // Detach wrappers while holding the lock
+ for (auto* w = socket_wrapper_list_.pop_front(); w != nullptr;
+ w = socket_wrapper_list_.pop_front())
+ socket_wrappers.push_back(w);
+
+ for (auto* w = acceptor_wrapper_list_.pop_front(); w != nullptr;
+ w = acceptor_wrapper_list_.pop_front())
+ acceptor_wrappers.push_back(w);
+ }
+
+ // Delete wrappers after releasing the lock to avoid deadlock.
+ for (auto* w : socket_wrappers)
+ delete w;
+ for (auto* w : acceptor_wrappers)
+ delete w;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::lock_guard<win_mutex> lock(mutex_); | |
| // Just close sockets and remove from list | |
| // The shared_ptrs held by socket objects and operations will handle destruction | |
| for (auto* impl = socket_list_.pop_front(); impl != nullptr; | |
| impl = socket_list_.pop_front()) | |
| { | |
| impl->close_socket(); | |
| delete impl; | |
| // Note: impl may still be alive if operations hold shared_ptr | |
| } | |
| for (auto* impl = acceptor_list_.pop_front(); impl != nullptr; | |
| impl = acceptor_list_.pop_front()) | |
| { | |
| impl->close_socket(); | |
| delete impl; | |
| } | |
| // Cleanup wrappers | |
| for (auto* w = socket_wrapper_list_.pop_front(); w != nullptr; | |
| w = socket_wrapper_list_.pop_front()) | |
| { | |
| delete w; | |
| } | |
| for (auto* w = acceptor_wrapper_list_.pop_front(); w != nullptr; | |
| w = acceptor_wrapper_list_.pop_front()) | |
| { | |
| delete w; | |
| } | |
| std::vector<win_socket_impl*> socket_wrappers; | |
| std::vector<win_acceptor_impl*> acceptor_wrappers; | |
| { | |
| std::lock_guard<win_mutex> lock(mutex_); | |
| // Just close sockets and remove from list | |
| // The shared_ptrs held by socket objects and operations will handle destruction | |
| for (auto* impl = socket_list_.pop_front(); impl != nullptr; | |
| impl = socket_list_.pop_front()) | |
| { | |
| impl->close_socket(); | |
| // Note: impl may still be alive if operations hold shared_ptr | |
| } | |
| for (auto* impl = acceptor_list_.pop_front(); impl != nullptr; | |
| impl = acceptor_list_.pop_front()) | |
| { | |
| impl->close_socket(); | |
| } | |
| // Detach wrappers while holding the lock | |
| for (auto* w = socket_wrapper_list_.pop_front(); w != nullptr; | |
| w = socket_wrapper_list_.pop_front()) | |
| socket_wrappers.push_back(w); | |
| for (auto* w = acceptor_wrapper_list_.pop_front(); w != nullptr; | |
| w = acceptor_wrapper_list_.pop_front()) | |
| acceptor_wrappers.push_back(w); | |
| } | |
| // Delete wrappers after releasing the lock to avoid deadlock. | |
| for (auto* w : socket_wrappers) | |
| delete w; | |
| for (auto* w : acceptor_wrappers) | |
| delete w; |
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/iocp/sockets.cpp` around lines 619 - 647, Holding
mutex_ while deleting wrapper objects (socket_wrapper_list_ and
acceptor_wrapper_list_) can deadlock if their destructors call unregister_impl()
which re-enters the same mutex; instead, under mutex_ move or pop the wrapper
pointers into local containers (e.g., std::vector or list) and release the lock,
then iterate those local containers and delete each wrapper outside the lock;
update the cleanup loop that currently deletes w from socket_wrapper_list_ and
acceptor_wrapper_list_ so it only collects pointers under mutex_ and performs
deletion after unlocking to avoid re-entrant mutex acquisition in
unregister_impl().
| void | ||
| testCancelAccept() | ||
| { | ||
| // Tests that cancel() properly cancels a pending accept operation. | ||
| // This exercises the acceptor_ptr shared_ptr that keeps the | ||
| // acceptor impl alive until IOCP delivers the cancellation. | ||
| io_context ioc; | ||
| acceptor acc(ioc); | ||
| acc.listen(endpoint(0)); | ||
|
|
||
| // These must outlive the coroutines | ||
| bool accept_done = false; | ||
| system::error_code accept_ec; | ||
| socket peer(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [&]() -> capy::task<> | ||
| { | ||
| // Start a timer to cancel the accept | ||
| timer t(ioc); | ||
| t.expires_after(std::chrono::milliseconds(50)); | ||
|
|
||
| // Launch accept that will block (no incoming connections) | ||
| // Store lambda in variable to ensure it outlives the coroutine. | ||
| auto nested_coro = [&acc, &peer, &accept_done, &accept_ec]() -> capy::task<> | ||
| { | ||
| auto [ec] = co_await acc.accept(peer); | ||
| accept_ec = ec; | ||
| accept_done = true; | ||
| }; | ||
| capy::run_async(ioc.get_executor())(nested_coro()); | ||
|
|
||
| // Wait for timer then cancel | ||
| co_await t.wait(); | ||
| acc.cancel(); | ||
|
|
||
| // Wait for accept to complete | ||
| timer t2(ioc); | ||
| t2.expires_after(std::chrono::milliseconds(50)); | ||
| co_await t2.wait(); | ||
|
|
||
| BOOST_TEST(accept_done); | ||
| BOOST_TEST(accept_ec == capy::cond::canceled); | ||
| }()); |
There was a problem hiding this comment.
Avoid fixed 50ms waits in cancellation tests.
These tests rely on a fixed 50ms delay before asserting accept_done, which can be flaky on slower CI or under sanitizers if cancellation delivery takes longer. Prefer awaiting the accept completion (or signaling completion explicitly) instead of sleeping a fixed interval; if you need a timeout, use a generous overall timeout while still waiting on completion.
Also applies to: 143-188
🤖 Prompt for AI Agents
In `@test/unit/acceptor.cpp` around lines 94 - 137, The test uses fixed 50ms
sleeps (timer t2) to wait for the accept coroutine to finish in testCancelAccept
(and similarly in the block on lines 143-188); instead of sleeping, capture the
async launch of nested_coro by storing the capy::run_async(...) call (e.g. auto
accept_task = capy::run_async(ioc.get_executor())(nested_coro());) and then
co_await accept_task to wait deterministically for the accept to complete
(optionally wrap that await in a generous overall timeout if you must bound the
test); remove the t2 timer and the fixed sleeps and replace them with awaiting
accept_task, and apply the same change to the other similar test block.
2 participants
Summary by CodeRabbit
New Features
Tests
Breaking Changes
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.