Skip to content

test stream write cancellation support and beast2::write tests #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
+531 −63
Draft

test stream write cancellation support and beast2::write tests #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa893d7
test stream write cancellation support and beast2::write tests
MungoG Nov 25, 2025
4c8060d
Merge branch 'cppalliance:develop' into issue-89-test-stream-cancella…
MungoG Nov 25, 2025
303cb20
rts rename to capy
MungoG Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/boost/beast2/impl/write.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ class write_some_op

BOOST_ASIO_CORO_REENTER(*this)
{
self.reset_cancellation_state(
asio::enable_total_cancellation());

rv = sr_.prepare();
if(! rv)
{
Expand Down Expand Up @@ -82,6 +85,9 @@ class write_some_op
}
sr_.consume(bytes_transferred);

if(!!self.cancelled() && !sr_.is_done())
Copy link
Collaborator

@ashtum ashtum Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition seems unnecessary, since we are not looping for another write, it doesn't matter whether async_write_some completes after the cancellation or before it.

Copy link
Collaborator Author

@MungoG MungoG Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it because, without this check, the following test does not produce the expected value of ec:
https://github.com/cppalliance/beast2/pull/102/files#diff-13a376592ec7d2e701ab2d597d68a2344c77761e8020e060f4eafdc2a1b39ea1R166-R200

Copy link
Collaborator

@ashtum ashtum Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link doesn’t work (it doesn’t point to a line number), but I guess the issue is related to the fact that async_write_some on test::stream completes immediately and there’s currently no way to prevent that. If that’s the case, we need a mechanism in test::stream to limit the read buffer so that even the first async_write_some doesn’t complete immediately.

ec = asio::error::operation_aborted;

upcall:
self.complete(
ec, bytes_transferred );
Expand Down Expand Up @@ -128,6 +134,10 @@ class write_op
dest_, sr_, std::move(self));
}
n_ += bytes_transferred;

if(!!self.cancelled() && ! sr_.is_done())
Copy link
Collaborator

@ashtum ashtum Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition should go before BOOST_ASIO_CORO_YIELD (the first thing that happens inside the loop). That way you don't need to check sr_.is_done(). Also, the current code swallows network errors if a cancellation happens afterwards, which is incorrect.

Copy link
Collaborator Author

@MungoG MungoG Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about the network errors, thank you.
If we emit a cancellation immediately after async_write what is the correct behaviour? Currently it will write some bytes and then the handler is called with non-zero n and an operation_aborted value of ec. Moving this to the top of the loop means that scenario will not write anything and n will be zero.

Copy link
Collaborator

@ashtum ashtum Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If asycn_write_asome completes before cancellation it would be caught in the next attempt to write, if asycn_write_asome completes with operation_aborted then the loop exits because of if(ec.failed) branch.

ec = asio::error::operation_aborted;

if(ec.failed())
break;
}
Expand Down
32 changes: 21 additions & 11 deletions include/boost/beast2/test/detail/stream_state.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ struct stream_read_op_base

//------------------------------------------------------------------------------

struct stream_write_op_base
Copy link
Collaborator

@ashtum ashtum Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this change to op_base and used for both read and write ops?

{
virtual ~stream_write_op_base() = default;
virtual void
operator()(system::error_code ec) = 0;
};

//------------------------------------------------------------------------------

enum class stream_status
{
ok,
Expand All @@ -89,8 +98,9 @@ struct stream_state
std::mutex m;
std::string storage;
buffers::string_buffer b;
std::condition_variable cv;
std::unique_ptr<stream_read_op_base> op;
//std::condition_variable cv;
std::unique_ptr<stream_read_op_base> rop;
std::unique_ptr<stream_write_op_base> wop;
stream_status code = stream_status::ok;
fail_count* fc = nullptr;
std::size_t nread = 0;
Expand Down Expand Up @@ -139,7 +149,7 @@ shutdown()
for(auto p : sp_->v_)
{
std::lock_guard<std::mutex> g2(p->m);
v.emplace_back(std::move(p->op));
v.emplace_back(std::move(p->rop));
p->code = detail::stream_status::eof;
}
}
Expand Down Expand Up @@ -200,8 +210,11 @@ stream_state::
~stream_state()
{
// cancel outstanding read
if(op != nullptr)
(*op)(asio::error::operation_aborted);
if(rop != nullptr)
(*rop)(asio::error::operation_aborted);
// cancel outstanding write
if(wop != nullptr)
(*wop)(asio::error::operation_aborted);
}

inline
Expand All @@ -223,16 +236,13 @@ void
stream_state::
notify_read()
{
if(op)
if(rop)
{
auto op_ = std::move(op);
auto op_ = std::move(rop);
op_->operator()(system::error_code{});
}
else
{
cv.notify_all();
}
}

} // detail
} // test
} // beast2
Expand Down
Loading
Loading