From 27196c40686c30b134952e9c1807f9f8b29be36d Mon Sep 17 00:00:00 2001 From: Niels Sascha Reedijk Date: Sun, 4 Sep 2022 07:27:08 +0100 Subject: [PATCH] NetServices: use BBorrow for custom body targets Change-Id: Ib2d4b0ca3689338d906f943295278c086c6f2c83 --- docs/user/netservices/HttpResult.dox | 19 ++++----- docs/user/netservices/HttpSession.dox | 8 ++-- headers/private/netservices2/HttpResult.h | 3 +- headers/private/netservices2/HttpSession.h | 3 +- .../libnetservices2/HttpResultPrivate.h | 31 +++++++++----- .../network/libnetservices2/HttpSession.cpp | 16 +++---- .../net/netservices2/HttpProtocolTest.cpp | 42 +++++++++++++++---- .../kits/net/netservices2/HttpProtocolTest.h | 27 ++++++------ 8 files changed, 91 insertions(+), 58 deletions(-) diff --git a/docs/user/netservices/HttpResult.dox b/docs/user/netservices/HttpResult.dox index 1b31db2cbd..2cc3d01008 100644 --- a/docs/user/netservices/HttpResult.dox +++ b/docs/user/netservices/HttpResult.dox @@ -114,9 +114,8 @@ namespace Network { \brief Represents a HTTP response body. The HTTP response body is captured in this object. The body is either stored into a - \ref target, or into a \a text variable, depending on how you called the - \ref BHttpSession::Execute() method. If there is a \a target, the body will be empty, - and vice versa. + target, or into a \ref text variable, depending on how you called the + \ref BHttpSession::Execute() method. You will usually get a reference to this object through the \ref BHttpResult::Body() method. If you want to keep the contents of the body beyond the lifetime of the BHttpResult object, @@ -127,17 +126,13 @@ namespace Network { /*! - \var std::unique_ptr BHttpBody::target - \brief An owned pointer to where the body has been written. - - \since Haiku R1 -*/ - - -/*! - \var BString BHttpBody::text + \var std::optional BHttpBody::text \brief A string containing the body of the HTTP request. + The value of this class variable is set to \c std::nullopt if the target body was written to + a specified target. Otherwise, the response body is stored in this string. If the response + body was empty, then this will be an empty string. + \since Haiku R1 */ diff --git a/docs/user/netservices/HttpSession.dox b/docs/user/netservices/HttpSession.dox index f2e8239e7b..b08fe1f501 100644 --- a/docs/user/netservices/HttpSession.dox +++ b/docs/user/netservices/HttpSession.dox @@ -165,14 +165,16 @@ namespace Network { /*! \fn BHttpResult BHttpSession::Execute(BHttpRequest &&request, - std::unique_ptr< BDataIO > target=nullptr, BMessenger observer=BMessenger()) + BBorrow< BDataIO > target=nullptr, BMessenger observer=BMessenger()) \brief Schedule and execute a \a request. \param request The (valid) request to move from. \param target An optional data buffer to write the incoming body of the request to. This can be \c nullptr if you want to use the default internal storage. If you provide a buffer, it - must be wrapped in a \c std::unique_ptr. This means that you transfer ownership to the - session. After the request is finished, you can regain ownership. + must be wrapped in a \ref BBorrow object. This means that you exclusively borrow the + target to this session object. After the request is finished, you can regain usage of the + object through the matching \ref BExclusiveBorrow object. Use the \ref BHttpResult::Body() + method to synchronize when the target is available again. \param observer An optional observer that will receive the progress and status messages for this request. diff --git a/headers/private/netservices2/HttpResult.h b/headers/private/netservices2/HttpResult.h index cd2f75d1af..fc8a290e92 100644 --- a/headers/private/netservices2/HttpResult.h +++ b/headers/private/netservices2/HttpResult.h @@ -23,8 +23,7 @@ struct HttpResultPrivate; struct BHttpBody { - std::unique_ptr target = nullptr; - BString text; + std::optional text; }; diff --git a/headers/private/netservices2/HttpSession.h b/headers/private/netservices2/HttpSession.h index 5f06d9f323..4869c4b79f 100644 --- a/headers/private/netservices2/HttpSession.h +++ b/headers/private/netservices2/HttpSession.h @@ -8,6 +8,7 @@ #include +#include #include class BUrl; @@ -35,7 +36,7 @@ public: // Requests BHttpResult Execute(BHttpRequest&& request, - std::unique_ptr target = nullptr, + BBorrow target = nullptr, BMessenger observer = BMessenger()); void Cancel(int32 identifier); void Cancel(const BHttpResult& request); diff --git a/src/kits/network/libnetservices2/HttpResultPrivate.h b/src/kits/network/libnetservices2/HttpResultPrivate.h index 11350e483d..56cb3d8fb1 100644 --- a/src/kits/network/libnetservices2/HttpResultPrivate.h +++ b/src/kits/network/libnetservices2/HttpResultPrivate.h @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -45,10 +46,9 @@ struct HttpResultPrivate { std::optional body; std::optional error; - // Body storage - std::unique_ptr ownedBody = nullptr; - // std::shared_ptr shared_body = nullptr; - BString bodyText; + // Interim body storage (used while the request is running) + BString bodyString; + BBorrow bodyTarget; // Utility functions HttpResultPrivate(int32 identifier); @@ -98,6 +98,9 @@ HttpResultPrivate::SetCancel() inline void HttpResultPrivate::SetError(std::exception_ptr e) { + // Release any held body target borrow + bodyTarget.Return(); + error = e; atomic_set(&requestStatus, kError); release_sem(data_wait); @@ -125,7 +128,12 @@ HttpResultPrivate::SetFields(BHttpFields&& f) inline void HttpResultPrivate::SetBody() { - body = BHttpBody{std::move(ownedBody), std::move(bodyText)}; + if (bodyTarget.HasValue()) { + body = BHttpBody{}; + bodyTarget.Return(); + } else + body = BHttpBody{std::move(bodyString)}; + atomic_set(&requestStatus, kBodyReady); release_sem(data_wait); } @@ -136,14 +144,15 @@ HttpResultPrivate::WriteToBody(const void* buffer, size_t size) { // TODO: when the support for a shared BMemoryRingIO is here, choose // between one or the other depending on which one is available. - if (ownedBody == nullptr) { - bodyText.Append(static_cast(buffer), size); + if (bodyTarget.HasValue()) { + auto result = bodyTarget->Write(buffer, size); + if (result < 0) + throw BSystemError("BDataIO::Write()", result); + return result; + } else { + bodyString.Append(reinterpret_cast(buffer), size); return size; } - auto result = ownedBody->Write(buffer, size); - if (result < 0) - throw BSystemError("BDataIO::Write()", result); - return result; } diff --git a/src/kits/network/libnetservices2/HttpSession.cpp b/src/kits/network/libnetservices2/HttpSession.cpp index b24ce745eb..2c149ad0bf 100644 --- a/src/kits/network/libnetservices2/HttpSession.cpp +++ b/src/kits/network/libnetservices2/HttpSession.cpp @@ -61,7 +61,7 @@ struct CounterDeleter { class BHttpSession::Request { public: Request(BHttpRequest&& request, - std::unique_ptr target, + BBorrow target, BMessenger observer); Request(Request& original, const Redirect& redirect); @@ -142,7 +142,7 @@ public: ~Impl() noexcept; BHttpResult Execute(BHttpRequest&& request, - std::unique_ptr target, + BBorrow target, BMessenger observer); void Cancel(int32 identifier); void SetMaxConnectionsPerHost(size_t maxConnections); @@ -232,7 +232,7 @@ BHttpSession::Impl::~Impl() noexcept BHttpResult -BHttpSession::Impl::Execute(BHttpRequest&& request, std::unique_ptr target, +BHttpSession::Impl::Execute(BHttpRequest&& request, BBorrow target, BMessenger observer) { auto wRequest = Request(std::move(request), std::move(target), observer); @@ -635,7 +635,7 @@ BHttpSession::operator=(const BHttpSession&) noexcept = default; BHttpResult -BHttpSession::Execute(BHttpRequest&& request, std::unique_ptr target, BMessenger observer) +BHttpSession::Execute(BHttpRequest&& request, BBorrow target, BMessenger observer) { return fImpl->Execute(std::move(request), std::move(target), observer); } @@ -670,8 +670,7 @@ BHttpSession::SetMaxHosts(size_t maxConnections) // #pragma mark -- BHttpSession::Request (helpers) - -BHttpSession::Request::Request(BHttpRequest&& request, std::unique_ptr target, +BHttpSession::Request::Request(BHttpRequest&& request, BBorrow target, BMessenger observer) : fRequest(std::move(request)), fObserver(observer) { @@ -682,7 +681,10 @@ BHttpSession::Request::Request(BHttpRequest&& request, std::unique_ptr // create shared data fResult = std::make_shared(identifier); - fResult->ownedBody = std::move(target); + + // check if there is a target + if (target.HasValue()) + fResult->bodyTarget = std::move(target); // inform the parser when we do a HEAD request, so not to expect content if (fRequest.Method() == BHttpMethod::Head) diff --git a/src/tests/kits/net/netservices2/HttpProtocolTest.cpp b/src/tests/kits/net/netservices2/HttpProtocolTest.cpp index 83b984100e..22af2762b5 100644 --- a/src/tests/kits/net/netservices2/HttpProtocolTest.cpp +++ b/src/tests/kits/net/netservices2/HttpProtocolTest.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -23,6 +24,8 @@ #include using BPrivate::BDateTime; +using BPrivate::Network::BBorrow; +using BPrivate::Network::BExclusiveBorrow; using BPrivate::Network::BHttpFields; using BPrivate::Network::BHttpMethod; using BPrivate::Network::BHttpRequest; @@ -32,6 +35,7 @@ using BPrivate::Network::BHttpTime; using BPrivate::Network::BHttpTimeFormat; using BPrivate::Network::BNetworkRequestError; using BPrivate::Network::format_http_time; +using BPrivate::Network::make_exclusive_borrow; using BPrivate::Network::parse_http_time; using namespace std::literals; @@ -453,6 +457,7 @@ HttpIntegrationTest::AddTests(BTestSuite& parent) testCaller->addThread("HostAndNetworkFailTest", &HttpIntegrationTest::HostAndNetworkFailTest); testCaller->addThread("GetTest", &HttpIntegrationTest::GetTest); + testCaller->addThread("GetWithBufferTest", &HttpIntegrationTest::GetWithBufferTest); testCaller->addThread("HeadTest", &HttpIntegrationTest::HeadTest); testCaller->addThread("NoContentTest", &HttpIntegrationTest::NoContentTest); testCaller->addThread("AutoRedirectTest", &HttpIntegrationTest::AutoRedirectTest); @@ -477,6 +482,7 @@ HttpIntegrationTest::AddTests(BTestSuite& parent) testCaller->addThread("HostAndNetworkFailTest", &HttpIntegrationTest::HostAndNetworkFailTest); testCaller->addThread("GetTest", &HttpIntegrationTest::GetTest); + testCaller->addThread("GetWithBufferTest", &HttpIntegrationTest::GetWithBufferTest); testCaller->addThread("HeadTest", &HttpIntegrationTest::HeadTest); testCaller->addThread("NoContentTest", &HttpIntegrationTest::NoContentTest); testCaller->addThread("AutoRedirectTest", &HttpIntegrationTest::AutoRedirectTest); @@ -559,7 +565,25 @@ HttpIntegrationTest::GetTest() CPPUNIT_ASSERT_EQUAL(field.Value(), (*expectedField).Value()); } auto receivedBody = result.Body().text; - CPPUNIT_ASSERT_EQUAL(kExpectedGetBody, receivedBody.String()); + CPPUNIT_ASSERT(receivedBody.has_value()); + CPPUNIT_ASSERT_EQUAL(kExpectedGetBody, receivedBody.value().String()); + } catch (const BPrivate::Network::BError& e) { + CPPUNIT_FAIL(e.DebugMessage().String()); + } +} + + +void +HttpIntegrationTest::GetWithBufferTest() +{ + auto request = BHttpRequest(BUrl(fTestServer.BaseUrl(), "/")); + auto body = make_exclusive_borrow(); + auto result = fSession.Execute(std::move(request), BBorrow(body), fLoggerMessenger); + try { + result.Body(); + auto bodyString = std::string(reinterpret_cast(body->Buffer()), + body->BufferLength()); + CPPUNIT_ASSERT_EQUAL(kExpectedGetBody, bodyString); } catch (const BPrivate::Network::BError& e) { CPPUNIT_FAIL(e.DebugMessage().String()); } @@ -584,8 +608,7 @@ HttpIntegrationTest::HeadTest() CPPUNIT_ASSERT_EQUAL(field.Value(), (*expectedField).Value()); } - auto receivedBody = result.Body().text; - CPPUNIT_ASSERT_EQUAL(receivedBody.Length(), 0); + CPPUNIT_ASSERT(result.Body().text->Length() == 0); } catch (const BPrivate::Network::BError& e) { CPPUNIT_FAIL(e.DebugMessage().String()); } @@ -618,8 +641,7 @@ HttpIntegrationTest::NoContentTest() CPPUNIT_ASSERT_EQUAL(field.Value(), (*expectedField).Value()); } - auto receivedBody = result.Body().text; - CPPUNIT_ASSERT_EQUAL(receivedBody.Length(), 0); + CPPUNIT_ASSERT(result.Body().text->Length() == 0); } catch (const BPrivate::Network::BError& e) { CPPUNIT_FAIL(e.DebugMessage().String()); } @@ -644,7 +666,8 @@ HttpIntegrationTest::AutoRedirectTest() CPPUNIT_ASSERT_EQUAL(field.Value(), (*expectedField).Value()); } auto receivedBody = result.Body().text; - CPPUNIT_ASSERT_EQUAL(kExpectedGetBody, receivedBody.String()); + CPPUNIT_ASSERT(receivedBody.has_value()); + CPPUNIT_ASSERT_EQUAL(kExpectedGetBody, receivedBody.value().String()); } catch (const BPrivate::Network::BError& e) { CPPUNIT_FAIL(e.DebugMessage().String()); } @@ -681,7 +704,7 @@ HttpIntegrationTest::StopOnErrorTest() auto result = fSession.Execute(std::move(request), nullptr, fLoggerMessenger); CPPUNIT_ASSERT(result.Status().code == 400); CPPUNIT_ASSERT(result.Fields().CountFields() == 0); - CPPUNIT_ASSERT(result.Body().text.Length() == 0); + CPPUNIT_ASSERT(result.Body().text->Length() == 0); } @@ -764,8 +787,9 @@ HttpIntegrationTest::PostTest() auto result = fSession.Execute(std::move(request), nullptr, BMessenger(observer)); - CPPUNIT_ASSERT_EQUAL(kExpectedPostBody.Length(), result.Body().text.Length()); - CPPUNIT_ASSERT(result.Body().text == kExpectedPostBody); + CPPUNIT_ASSERT(result.Body().text.has_value()); + CPPUNIT_ASSERT_EQUAL(kExpectedPostBody.Length(), result.Body().text.value().Length()); + CPPUNIT_ASSERT(result.Body().text.value() == kExpectedPostBody); usleep(2000); // give some time to catch up on receiving all messages diff --git a/src/tests/kits/net/netservices2/HttpProtocolTest.h b/src/tests/kits/net/netservices2/HttpProtocolTest.h index aa35196608..96a39f8156 100644 --- a/src/tests/kits/net/netservices2/HttpProtocolTest.h +++ b/src/tests/kits/net/netservices2/HttpProtocolTest.h @@ -33,22 +33,23 @@ public: class HttpIntegrationTest : public BThreadedTestCase { public: - HttpIntegrationTest(TestServerMode mode); + HttpIntegrationTest(TestServerMode mode); - virtual void setUp() override; - virtual void tearDown() override; + virtual void setUp() override; + virtual void tearDown() override; - void HostAndNetworkFailTest(); - void GetTest(); - void HeadTest(); - void NoContentTest(); - void AutoRedirectTest(); - void BasicAuthTest(); - void StopOnErrorTest(); - void RequestCancelTest(); - void PostTest(); + void HostAndNetworkFailTest(); + void GetTest(); + void GetWithBufferTest(); + void HeadTest(); + void NoContentTest(); + void AutoRedirectTest(); + void BasicAuthTest(); + void StopOnErrorTest(); + void RequestCancelTest(); + void PostTest(); - static void AddTests(BTestSuite& suite); + static void AddTests(BTestSuite& suite); private: TestServer fTestServer;