From b2e7d3afa4a0b91ab04b2482e5c07e53fd540b59 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Sat, 23 Dec 2023 15:07:09 -0800 Subject: [PATCH 1/2] fix: elastic buffer semantics in parser --- .../boost/http_proto/detail/type_traits.hpp | 77 ++++++++++++++++++ include/boost/http_proto/impl/parser.hpp | 55 +++++++++++-- include/boost/http_proto/parser.hpp | 53 +++++++++--- test/unit/parser.cpp | 80 +++++++++++++++++-- 4 files changed, 237 insertions(+), 28 deletions(-) create mode 100644 include/boost/http_proto/detail/type_traits.hpp diff --git a/include/boost/http_proto/detail/type_traits.hpp b/include/boost/http_proto/detail/type_traits.hpp new file mode 100644 index 00000000..85ed444b --- /dev/null +++ b/include/boost/http_proto/detail/type_traits.hpp @@ -0,0 +1,77 @@ +// +// Copyright (c) 2023 Vinnie Falco (vinnie.falco@gmail.com) +// +// Distributed under the Boost Software License, Version 1.0. (See accompanying +// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) +// +// Official repository: https://github.com/cppalliance/http_proto +// + +#ifndef BOOST_HTTP_PROTO_DETAIL_TYPE_TRAITS_HPP +#define BOOST_HTTP_PROTO_DETAIL_TYPE_TRAITS_HPP + +#include +#include + +namespace boost { +namespace http_proto { +namespace detail { + +template +struct remove_cvref +{ + using type = + typename std::remove_cv< + typename std::remove_reference::type>::type; +}; + +template +using remove_cvref_t = typename remove_cvref::type; + +template +struct is_reference_wrapper_impl + : std::false_type +{ +}; + +template +struct is_reference_wrapper_impl< + std::reference_wrapper> + : std::true_type +{ +}; + +#if !defined(BOOST_NO_CV_SPECIALIZATIONS) + +template +struct is_reference_wrapper_impl< + std::reference_wrapper const> + : std::true_type +{ +}; + +template +struct is_reference_wrapper_impl< + std::reference_wrapper volatile> + : std::true_type +{ +}; + +template +struct is_reference_wrapper_impl< + std::reference_wrapper const volatile> + : std::true_type +{ +}; + +#endif + +template +using is_reference_wrapper = + is_reference_wrapper_impl>; + +} // detail +} // http_proto +} // boost + +#endif diff --git a/include/boost/http_proto/impl/parser.hpp b/include/boost/http_proto/impl/parser.hpp index 6f165e1b..0e83b4c2 100644 --- a/include/boost/http_proto/impl/parser.hpp +++ b/include/boost/http_proto/impl/parser.hpp @@ -17,13 +17,27 @@ namespace http_proto { //------------------------------------------------ -template -typename std::decay< - DynamicBuffer>::type& +template +typename std::enable_if< + ! detail::is_reference_wrapper< + ElasticBuffer>::value && + ! is_sink::value>::type parser:: set_body( - DynamicBuffer&& b) + ElasticBuffer&& eb) { + // If this goes off it means you are trying + // to pass by lvalue reference. Use std::ref + // instead. + static_assert( + ! std::is_reference::value, + "Use std::ref instead of pass-by-reference"); + + // Check ElasticBuffer type requirements + static_assert( + buffers::is_dynamic_buffer::value, + "Type requirements not met."); + // body must not be set already if(how_ != how::in_place) detail::throw_logic_error(); @@ -34,13 +48,40 @@ set_body( auto& dyn = ws_.push( buffers::any_dynamic_buffer_impl::type, + std::decay::type, buffers_N>(std::forward< - DynamicBuffer>(b))); + ElasticBuffer>(eb))); + dyn_ = &dyn; + how_ = how::dynamic; + on_set_body(); +} + +template +void +parser:: +set_body( + std::reference_wrapper eb) +{ + // Check ElasticBuffer type requirements + static_assert( + buffers::is_dynamic_buffer::value, + "Type requirements not met."); + + // body must not be set already + if(how_ != how::in_place) + detail::throw_logic_error(); + + // headers must be complete + if(! got_header()) + detail::throw_logic_error(); + + auto& dyn = ws_.push( + buffers::any_dynamic_buffer_impl::type&, + buffers_N>(eb)); dyn_ = &dyn; how_ = how::dynamic; on_set_body(); - return dyn.buffer(); } //------------------------------------------------ diff --git a/include/boost/http_proto/parser.hpp b/include/boost/http_proto/parser.hpp index a10777b1..7a00b22f 100644 --- a/include/boost/http_proto/parser.hpp +++ b/include/boost/http_proto/parser.hpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,7 @@ #include #include #include +#include #include #include @@ -258,36 +260,60 @@ class BOOST_SYMBOL_VISIBLE /** Parse pending input data */ + // VFALCO return result? BOOST_HTTP_PROTO_DECL void parse( system::error_code& ec); - /** Attach a body + /** Attach a body. + + This function attaches the specified elastic + buffer as the storage for the message body. + The parser acquires ownership of the object + `eb` and destroys it when: + + @li @ref is_complete returns `true`, or + @li @ref reset is called, or + @li an unrecoverable parsing error occurs, or + @li the parser is destroyed. */ // VFALCO Should this function have // error_code& ec and call parse? + template #ifndef BOOST_HTTP_PROTO_DOCS - template< - class DynamicBuffer - , class = typename std::enable_if< - buffers::is_dynamic_buffer< - DynamicBuffer>::value - >::type - > + typename std::enable_if< + ! detail::is_reference_wrapper< + ElasticBuffer>::value && + ! is_sink::value>::type #else - template + void #endif - typename std::decay< - DynamicBuffer>::type& - set_body(DynamicBuffer&& b); + set_body(ElasticBuffer&& eb); + + /** Attach a body. + + This function attaches the specified elastic + buffer reference as the storage for the message body. + Ownership is not transferred; the caller must + ensure that the lifetime of the object + reference by `eb` extends until: + + @li @ref is_complete returns `true`, or + @li @ref reset is called, or + @li an unrecoverable parsing error occurs, or + @li the parser is destroyed. + */ + template + void set_body( + std::reference_wrapper eb); /** Attach a body */ template #ifndef BOOST_HTTP_PROTO_DOCS typename std::enable_if< - is_sink::value, + is_sink::value, typename std::decay::type >::type& #else @@ -321,6 +347,7 @@ class BOOST_SYMBOL_VISIBLE could be additional protocol-dependent data that we want to retrieve. */ + // VFALCO rename to get_leftovers()? BOOST_HTTP_PROTO_DECL core::string_view release_buffered_data() noexcept; diff --git a/test/unit/parser.cpp b/test/unit/parser.cpp index 29f70460..fa81549a 100644 --- a/test/unit/parser.cpp +++ b/test/unit/parser.cpp @@ -25,6 +25,73 @@ #include +//------------------------------------------------ +/* + +Parser operation + + For caller provided objects the parser can copy + its internal contents into the caller's object + by calling a function. Or it can request a buffer + from the caller's object into which the body + contents are placed. In the simple case this + means that enclosing socket reads can read + directly into caller-provided buffers. + +General Case + parser pr; + error_code ec; + + pr.start(); // must call first + auto mb = pr.prepare(); // returns the input buffer + ... + pr.commit( n ); // commit input buffer bytes + pr.parse( ec ); // parse data + // (inspect ec for special codes) + +Parser-provided string_view body (default) + + // nothing to do + assert( pr.is_complete() ); + string_view s = pr.body(); + +Parser-provided buffer-at-time body + + parser::const_buffers_type part = pr.pull_some() + +Caller-provided body buffers (dynamic buffer?) + + pr.set_body( bb ); // + +Caller-provided sink + + pr.set_body( sk ); + +-------------------------------------------------- + +Caller wants to parse a message and have the body +stored in a std::string upon completion. + 1. Re-using an existing string, or + 2. Creating a new string + +This all speaks to DynamicBuffer as the correct API + * But is our DynamicBuffer owning or reference-like? + * What triggers the final resize() on the std::string? + - destructor + - other member function + + parser pr; + std::string s; + ... + pr.set_body( buffers::dynamic_for( s ) ); // reference semantics + + parser pr; + buffers::flat_buffer fb; + ... + pr.set_body( fb ); // flat_buffer& +*/ +//------------------------------------------------ + namespace boost { namespace http_proto { @@ -600,13 +667,11 @@ struct parser_test // requires small string optimization BOOST_TEST_GT(s.capacity(), 0); BOOST_TEST_LT(s.capacity(), 5000); - auto& body = pr.set_body( - buffers::string_buffer(&s)); + pr.set_body(buffers::string_buffer(&s)); auto dest = pr.prepare(); - BOOST_TEST_EQ( + BOOST_TEST_LE( buffers::buffer_size(dest), - body.capacity()); - + s.capacity()); pr.reset(); } @@ -1205,9 +1270,8 @@ struct parser_test BOOST_TEST_EQ(ec, ex); return; } - auto& fb = pr_->set_body( - buffers::flat_buffer( - buf, sizeof(buf))); + buffers::flat_buffer fb(buf, sizeof(buf)); + pr_->set_body(std::ref(fb)); BOOST_TEST(pr_->body().empty()); if(! pr_->is_complete()) { From 7314afe8ba58b5ea51aad48ec67bf4c0a020326b Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Sat, 23 Dec 2023 15:38:57 -0800 Subject: [PATCH 2/2] chore: rename to elastic --- include/boost/http_proto/impl/parser.hpp | 8 ++-- include/boost/http_proto/impl/parser.ipp | 61 +++++++++++++----------- include/boost/http_proto/parser.hpp | 4 +- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/include/boost/http_proto/impl/parser.hpp b/include/boost/http_proto/impl/parser.hpp index 0e83b4c2..10ea090b 100644 --- a/include/boost/http_proto/impl/parser.hpp +++ b/include/boost/http_proto/impl/parser.hpp @@ -51,8 +51,8 @@ set_body( std::decay::type, buffers_N>(std::forward< ElasticBuffer>(eb))); - dyn_ = &dyn; - how_ = how::dynamic; + eb_ = &dyn; + how_ = how::elastic; on_set_body(); } @@ -79,8 +79,8 @@ set_body( buffers::any_dynamic_buffer_impl::type&, buffers_N>(eb)); - dyn_ = &dyn; - how_ = how::dynamic; + eb_ = &dyn; + how_ = how::elastic; on_set_body(); } diff --git a/include/boost/http_proto/impl/parser.ipp b/include/boost/http_proto/impl/parser.ipp index 31c3fa0b..7c71b3cf 100644 --- a/include/boost/http_proto/impl/parser.ipp +++ b/include/boost/http_proto/impl/parser.ipp @@ -224,6 +224,7 @@ parser( parser_service>()) , h_(detail::empty{k}) , st_(state::reset) + , eb_(nullptr) { auto const n = svc_.space_needed; @@ -249,6 +250,12 @@ void parser:: reset() noexcept { + if(eb_) + { + eb_->~any_dynamic_buffer(); + eb_ = nullptr; + } + ws_.clear(); st_ = state::start; got_eof_ = false; @@ -404,7 +411,7 @@ prepare() -> return mutable_buffers_type(mbp_); } - if(how_ == how::dynamic) + if(how_ == how::elastic) { // Overreads are not allowed, or // else the caller will see extra @@ -419,7 +426,7 @@ prepare() -> if( n > svc_.cfg.max_prepare) n = svc_.cfg.max_prepare; nprepare_ = n; - return dyn_->prepare(n); + return eb_->prepare(n); } BOOST_ASSERT( @@ -434,8 +441,8 @@ prepare() -> { // apply max_size() auto avail = - dyn_->max_size() - - dyn_->size(); + eb_->max_size() - + eb_->size(); if( n > avail) n = avail; } @@ -443,8 +450,8 @@ prepare() -> // to avoid an allocation { auto avail = - dyn_->capacity() - - dyn_->size(); + eb_->capacity() - + eb_->size(); if( n > avail && avail != 0) n = avail; @@ -466,7 +473,7 @@ prepare() -> } } nprepare_ = n; - return dyn_->prepare(n); + return eb_->prepare(n); } // VFALCO TODO @@ -481,7 +488,7 @@ prepare() -> { BOOST_ASSERT(is_plain()); - if(how_ == how::dynamic) + if(how_ == how::elastic) { // attempt to transfer in-place // body into the dynamic buffer. @@ -604,14 +611,14 @@ commit( break; } - if(how_ == how::dynamic) + if(how_ == how::elastic) { - if(dyn_->size() < dyn_->max_size()) + if(eb_->size() < eb_->max_size()) { BOOST_ASSERT(body_avail_ == 0); BOOST_ASSERT( body_buf_->size() == 0); - dyn_->commit(n); + eb_->commit(n); } else { @@ -659,7 +666,7 @@ commit( BOOST_ASSERT(is_plain()); BOOST_ASSERT(n == 0); - if( how_ == how::dynamic || + if( how_ == how::elastic || how_ == how::sink) { // intended no-op @@ -865,7 +872,7 @@ parse( break; } - if(how_ == how::dynamic) + if(how_ == how::elastic) { // state already updated in commit if(h_.md.payload == payload::size) @@ -876,8 +883,8 @@ parse( if(body_avail_ != 0) { BOOST_ASSERT( - dyn_->max_size() - - dyn_->size() < + eb_->max_size() - + eb_->size() < payload_remain_); ec = BOOST_HTTP_PROTO_ERR( error::buffer_overflow); @@ -895,7 +902,7 @@ parse( } BOOST_ASSERT( h_.md.payload == payload::to_eof); - if( dyn_->size() == dyn_->max_size() && + if( eb_->size() == eb_->max_size() && body_avail_ > 0) { // got here from the 1-byte read @@ -924,7 +931,7 @@ parse( // transfer in_place data into set body - if(how_ == how::dynamic) + if(how_ == how::elastic) { init_dynamic(ec); if(! ec.failed()) @@ -1003,13 +1010,13 @@ parse( case how::in_place: break; - case how::dynamic: + case how::elastic: { if(body_buf_->size() == 0) break; - BOOST_ASSERT(dyn_->size() == 0); + BOOST_ASSERT(eb_->size() == 0); auto n = buffers::buffer_copy( - dyn_->prepare( + eb_->prepare( body_buf_->size()), body_buf_->data()); body_buf_->consume(n); @@ -1245,7 +1252,7 @@ on_set_body() nprepare_ = 0; // invalidate - if(how_ == how::dynamic) + if(how_ == how::elastic) { if(h_.md.payload == payload::none) { @@ -1288,7 +1295,7 @@ init_dynamic( BOOST_ASSERT( body_total_ == body_avail_); auto const space_left = - dyn_->max_size() - dyn_->size(); + eb_->max_size() - eb_->size(); if(h_.md.payload == payload::size) { @@ -1299,14 +1306,14 @@ init_dynamic( return; } // reserve the full size - dyn_->prepare(h_.md.payload_size); + eb_->prepare(h_.md.payload_size); // transfer in-place body auto n = body_avail_; if( n > h_.md.payload_size) n = h_.md.payload_size; - dyn_->commit( + eb_->commit( buffers::buffer_copy( - dyn_->prepare(n), + eb_->prepare(n), body_buf_->data())); BOOST_ASSERT(body_avail_ == n); BOOST_ASSERT(body_total_ == n); @@ -1334,9 +1341,9 @@ init_dynamic( error::buffer_overflow); return; } - dyn_->commit( + eb_->commit( buffers::buffer_copy( - dyn_->prepare(body_avail_), + eb_->prepare(body_avail_), body_buf_->data())); body_buf_->consume(body_avail_); body_avail_ = 0; diff --git a/include/boost/http_proto/parser.hpp b/include/boost/http_proto/parser.hpp index 7a00b22f..06ec071c 100644 --- a/include/boost/http_proto/parser.hpp +++ b/include/boost/http_proto/parser.hpp @@ -379,7 +379,7 @@ class BOOST_SYMBOL_VISIBLE enum class how { in_place, - dynamic, + elastic, sink, pull }; @@ -398,7 +398,7 @@ class BOOST_SYMBOL_VISIBLE buffers::circular_buffer cb1_; buffers::circular_buffer* body_buf_; buffers::mutable_buffer_pair mbp_; - buffers::any_dynamic_buffer* dyn_; + buffers::any_dynamic_buffer* eb_; filter* filt_; sink* sink_;