test stream write cancellation support and beast2::write tests #102
Conversation
|
GCOVR code coverage report https://102.beast2.prtest3.cppalliance.org/gcovr/index.html Build time: 2025-11-25 18:20:50 UTC |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #102 +/- ##
===========================================
+ Coverage 53.19% 56.96% +3.77%
===========================================
Files 38 38
Lines 1690 1808 +118
===========================================
+ Hits 899 1030 +131
+ Misses 791 778 -13
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #102 +/- ##
===========================================
+ Coverage 53.19% 56.96% +3.77%
===========================================
Files 38 38
Lines 1690 1808 +118
===========================================
+ Hits 899 1030 +131
+ Misses 791 778 -13
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| } | ||
| n_ += bytes_transferred; | ||
|
|
||
| if(!!self.cancelled() && ! sr_.is_done()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| sr_.consume(bytes_transferred); | ||
|
|
||
| if(!!self.cancelled() && !sr_.is_done()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
|
||
| //------------------------------------------------------------------------------ | ||
|
|
||
| struct stream_write_op_base |
There was a problem hiding this comment.
Could this change to op_base and used for both read and write ops?
| std::vector<std::unique_ptr<detail::stream_read_op_base>> v; | ||
| std::lock_guard<std::mutex> g1(sp_->m_); | ||
| v.reserve(sp_->v_.size()); | ||
| 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; | ||
| } |
There was a problem hiding this comment.
stream_service::shutdown() should also destroy hanging write operations on shutdown.
|
please flatten this branch (no merge commits) |
4 participants
No description provided.