From: Marcin Siodelski Date: Thu, 18 Jan 2018 17:24:51 +0000 (+0100) Subject: [5451] Addressed review comments. X-Git-Tag: trac5457_base~4^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=32d1e91a4c058333bac290d6bc9be70ab859af3e;p=thirdparty%2Fkea.git [5451] Addressed review comments. --- diff --git a/src/bin/agent/ca_response_creator.h b/src/bin/agent/ca_response_creator.h index 247d21c456..d8f1dd8ae7 100644 --- a/src/bin/agent/ca_response_creator.h +++ b/src/bin/agent/ca_response_creator.h @@ -59,6 +59,10 @@ private: /// @brief Creates unfinalized stock HTTP response. /// + /// The unfinilized response is the response that can't be sent over the + /// wire until @c finalize() is called, which commits the contents of the + /// message body. + /// /// @param request Pointer to an object representing HTTP request. /// @param status_code Status code of the response. /// @return Pointer to an @ref isc::http::HttpResponseJson object diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index 2691735312..7da68df029 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -25,9 +25,6 @@ using namespace http; namespace { -/// @brief Default request timeout of 10s. -const long REQUEST_TIMEOUT = 10000; - /// @brief TCP socket callback function type. typedef boost::function SocketCallbackFunction; @@ -119,9 +116,6 @@ public: void doTransaction(const HttpRequestPtr& request, const HttpResponsePtr& response, const long request_timeout, const HttpClient::RequestHandler& callback); - /// @brief Closes connection and removes it from the connection pool. - void stop(); - /// @brief Closes the socket and cancels the request timer. void close(); @@ -464,12 +458,6 @@ Connection::doTransaction(const HttpRequestPtr& request, } } -void -Connection::stop() { - ConnectionPoolPtr conn_pool = conn_pool_.lock(); - conn_pool->closeConnection(url_); -} - void Connection::close() { timer_.cancel(); @@ -668,17 +656,8 @@ HttpClient::HttpClient(IOService& io_service) void HttpClient::asyncSendRequest(const Url& url, const HttpRequestPtr& request, const HttpResponsePtr& response, - const HttpClient::RequestHandler& callback) { - asyncSendRequest(url, request, response, - HttpClient::RequestTimeout(REQUEST_TIMEOUT), - callback); -} - -void -HttpClient::asyncSendRequest(const Url& url, const HttpRequestPtr& request, - const HttpResponsePtr& response, - const HttpClient::RequestTimeout& request_timeout, - const HttpClient::RequestHandler& callback) { + const HttpClient::RequestHandler& callback, + const HttpClient::RequestTimeout& request_timeout) { if (!url.isValid()) { isc_throw(HttpClientError, "invalid URL specified for the HTTP client"); } diff --git a/src/lib/http/client.h b/src/lib/http/client.h index 943b19defb..a25d07a552 100644 --- a/src/lib/http/client.h +++ b/src/lib/http/client.h @@ -29,6 +29,40 @@ public: class HttpClientImpl; /// @brief HTTP client class. +/// +/// This class implements an asynchronous HTTP client. The caller can schedule +/// transmission of the HTTP request using @ref HttpClient::asyncSendRequest +/// method. The caller specifies target URL for each request. The caller also +/// specifies a pointer to the @ref HttpRequest or derived class, holding a +/// request that should be transmitted to the destination. Such request must +/// be finalized, i.e. @ref HttpRequest::finalize method must be called prior +/// to sending it. The caller must also provide a pointer to the +/// @ref HttpResponse object or an object derived from it. The type of the +/// response object must match the expected content type to be returned in the +/// server's response. The last argument specified in this call is the pointer +/// to the callback function, which should be launched when the response is +/// received, an error occurs or when a timeout in the transmission is +/// signalled. +/// +/// The HTTP client supports multiple simultaneous and persistent connections +/// with different destinations. The client determines if the connection is +/// persistent by looking into the Connection header and HTTP version of the +/// request. If the connection should be persistent the client doesn't +/// close the connection after sending a request and receiving a response from +/// the server. If the client is provided with the request to be sent to the +/// particular destination, but there is an ongoing communication with this +/// destination, e.g. as a result of sending previous request, the new +/// request is queued in the FIFO queue. When the previous request completes, +/// the next request in the queue for the particular URL will be initiated. +/// +/// The client tests the persistent connection for usability before sending +/// a request by trying to read from the socket (with message peeking). If +/// the socket is usable the client uses it to transmit the request. +/// +/// All errors are reported to the caller via the callback function supplied +/// to the @ref HttpClient::asyncSendRequest. The IO errors are communicated +/// via the @c boost::system::error code value. The response parsing errors +/// are returned via the 3rd parameter of the callback. class HttpClient { public: @@ -55,51 +89,54 @@ public: /// @brief Queues new asynchronous HTTP request. /// - /// The client creates one connection for the specified URL. If the connection - /// with the particular destination already exists, it will be re-used for the - /// new transaction scheduled with this call. If another transaction is still - /// in progress, the new transaction is queued. The queued transactions are - /// started in the FIFO order one after another. If the connection is idle or - /// the connection doesn't exist, the new transaction is started immediatelly. + /// The client creates one connection for the specified URL. If the + /// connection with the particular destination already exists, it will be + /// re-used for the new transaction scheduled with this call. If another + /// transaction is still in progress, the new transaction is queued. The + /// queued transactions are started in the FIFO order one after another. If + /// the connection is idle or the connection doesn't exist, the new + /// transaction is started immediatelly. /// - /// The existing connection is tested before it is used for the new transaction - /// by attempting to read (with message peeking) from the open TCP socket. If the - /// read attempt is successful, the client will transmit the HTTP request to - /// the server using this connection. It is possible that the server closes the - /// connection between the connection test and sending the request. In such case, - /// an error will be returned and the caller will need to try re-sending the - /// request. + /// The existing connection is tested before it is used for the new + /// transaction by attempting to read (with message peeking) from the open + /// TCP socket. If the read attempt is successful, the client will transmit + /// the HTTP request to the server using this connection. It is possible + /// that the server closes the connection between the connection test and + /// sending the request. In such case, an error will be returned and the + /// caller will need to try re-sending the request. /// - /// If the connection test fails, the client will close the socket and reconnect - /// to the server prior to sending the request. + /// If the connection test fails, the client will close the socket and + /// reconnect to the server prior to sending the request. /// - /// Pointers to the request and response objects are provided as arguments of - /// this method. These pointers should have appropriate types derived from the - /// @ref HttpRequest and @ref HttpResponse classes. For example, if the request - /// has content type "application/json", a pointer to the + /// Pointers to the request and response objects are provided as arguments + /// of this method. These pointers should have appropriate types derived + /// from the @ref HttpRequest and @ref HttpResponse classes. For example, + /// if the request has content type "application/json", a pointer to the /// @ref HttpResponseJson should be passed. In this case, the response type - /// should be @ref HttpResponseJson. These types are used to validate both the - /// request provided by the caller and the response received from the server. + /// should be @ref HttpResponseJson. These types are used to validate both + /// the request provided by the caller and the response received from the + /// server. /// - /// The callback function provided by the caller is invoked when the transaction - /// terminates, i.e. when the server has responded or when an error occured. The - /// callback is expected to be exception safe, but the client internally guards - /// against exceptions thrown by the callback. + /// The callback function provided by the caller is invoked when the + /// transaction terminates, i.e. when the server has responded or when an + /// error occured. The callback is expected to be exception safe, but the + /// client internally guards against exceptions thrown by the callback. /// - /// The first argument of the callback indicates an IO error during communication - /// with the server. If the communication is successful the error code of 0 is - /// returned. However, in this case it is still possible that the transaction is - /// unsuccessful due to HTTP response parsing error, e.g. invalid content type, - /// malformed response etc. Such errors are indicated via third argument. + /// The first argument of the callback indicates an IO error during + /// communication with the server. If the communication is successful the + /// error code of 0 is returned. However, in this case it is still possible + /// that the transaction is unsuccessful due to HTTP response parsing error, + /// e.g. invalid content type, malformed response etc. Such errors are + /// indicated via third argument. /// - /// If message parsing was successful the second argument of the callback contains - /// a pointer to the parsed response (the same pointer as provided by the caller - /// as the argument). If parsing was unsuccessful, the null pointer is returned. + /// If message parsing was successful the second argument of the callback + /// contains a pointer to the parsed response (the same pointer as provided + ///by the caller as the argument). If parsing was unsuccessful, the null + /// pointer is returned. /// - /// The default timeout for the transaction is set to 10 seconds (10 000 ms). This - /// variant of the method doesn't allow for specifying a custom timeout. If the - /// timeout occurs, the callback is invoked with the error code of - /// @c boost::asio::error::timed_out. + /// The default timeout for the transaction is set to 10 seconds + /// (10 000 ms). If the timeout occurs, the callback is invoked with the + //// error code of @c boost::asio::error::timed_out. /// /// @param url URL where the request should be send. /// @param request Pointer to the object holding a request. @@ -110,22 +147,9 @@ public: void asyncSendRequest(const Url& url, const HttpRequestPtr& request, const HttpResponsePtr& response, - const RequestHandler& callback); - - /// @brief Queues new asynchronous HTTP request with timeout. - /// - /// @param url URL where the request should be send. - /// @param request Pointer to the object holding a request. - /// @param response Pointer to the object where response should be stored. - /// @param request_timeout Timeout for the transaction in milliseconds. - /// @param callback Pointer to the user callback function. - /// - /// @throw HttpClientError If invalid arguments were provided. - void asyncSendRequest(const Url& url, - const HttpRequestPtr& request, - const HttpResponsePtr& response, - const RequestTimeout& request_timeout, - const RequestHandler& callback); + const RequestHandler& callback, + const RequestTimeout& request_timeout = + RequestTimeout(10000)); /// @brief Closes all connections. void stop(); diff --git a/src/lib/http/http_message.cc b/src/lib/http/http_message.cc index fd4e4bb9b2..0f7166dfed 100644 --- a/src/lib/http/http_message.cc +++ b/src/lib/http/http_message.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -56,20 +56,6 @@ HttpMessage::getHttpVersion() const { HttpHeaderPtr HttpMessage::getHeader(const std::string& header_name) const { - HttpHeaderPtr http_header = getHeaderSafe(header_name); - - // No such header. - if (!http_header) { - isc_throw(HttpMessageNonExistingHeader, header_name << " HTTP header" - " not found in the request"); - } - - // Header found. - return (http_header); -} - -HttpHeaderPtr -HttpMessage::getHeaderSafe(const std::string& header_name) const { checkCreated(); HttpHeader hdr(header_name); @@ -78,8 +64,8 @@ HttpMessage::getHeaderSafe(const std::string& header_name) const { return (header_it->second); } - // Header not found. Return null pointer. - return (HttpHeaderPtr()); + isc_throw(HttpMessageNonExistingHeader, header_name << " HTTP header" + " not found in the request"); } std::string diff --git a/src/lib/http/http_message.h b/src/lib/http/http_message.h index 61dfb99802..3634c80fa7 100644 --- a/src/lib/http/http_message.h +++ b/src/lib/http/http_message.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -162,19 +162,6 @@ public: /// @throw HttpMessageError if the request hasn't been created. HttpHeaderPtr getHeader(const std::string& header_name) const; - /// @brief Returns object encapsulating HTTP header. - /// - /// This variant doesn't throw an exception if the header doesn't exist. - /// It will throw if the request hasn't been created using @c create() - /// method. - /// - /// @param header_name HTTP header name. - /// - /// @return Pointer to the specified header, or null if such header doesn't - /// exist. - /// @throw HttpMessageError if the request hasn't been created. - HttpHeaderPtr getHeaderSafe(const std::string& header_name) const; - /// @brief Returns a value of the specified HTTP header. /// /// @param header_name Name of the HTTP header. diff --git a/src/lib/http/http_message_parser_base.cc b/src/lib/http/http_message_parser_base.cc index a7c01bf85f..a5d84c660e 100644 --- a/src/lib/http/http_message_parser_base.cc +++ b/src/lib/http/http_message_parser_base.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -143,7 +143,7 @@ HttpMessageParserBase::getNextFromBuffer() { // NEED_MORE_DATA_EVT it indicates that the caller hasn't provided // the data. if (ev == NEED_MORE_DATA_EVT) { - isc_throw(HttpMessageParserBaseError, + isc_throw(HttpParseError, "HTTP request parser requires new data to progress, but no data" " have been provided. The transaction is aborted to avoid" " a deadlock. This is a Kea HTTP server logic error!"); @@ -155,7 +155,7 @@ HttpMessageParserBase::getNextFromBuffer() { // There is no more data so it is really not possible that we're // at MORE_DATA_PROVIDED_EVT. if (ev == MORE_DATA_PROVIDED_EVT) { - isc_throw(HttpMessageParserBaseError, + isc_throw(HttpParseError, "HTTP server state indicates that new data have been" " provided to be parsed, but the transaction buffer" " contains no new data. This is a Kea HTTP server logic" @@ -174,7 +174,7 @@ HttpMessageParserBase::getNextFromBuffer() { void HttpMessageParserBase::invalidEventError(const std::string& handler_name, const unsigned int event) { - isc_throw(HttpMessageParserBaseError, handler_name << ": " + isc_throw(HttpParseError, handler_name << ": " << " invalid event " << getEventLabel(static_cast(event))); } diff --git a/src/lib/http/http_message_parser_base.h b/src/lib/http/http_message_parser_base.h index e909e8ff64..78e7640213 100644 --- a/src/lib/http/http_message_parser_base.h +++ b/src/lib/http/http_message_parser_base.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -21,9 +21,9 @@ namespace http { /// has occurred. /// /// The most common errors are due to receiving malformed requests. -class HttpMessageParserBaseError : public Exception { +class HttpParseError : public Exception { public: - HttpMessageParserBaseError(const char* file, size_t line, const char* what) : + HttpParseError(const char* file, size_t line, const char* what) : isc::Exception(file, line, what) { }; }; diff --git a/src/lib/http/request.cc b/src/lib/http/request.cc index 65847f8343..678f8b605d 100644 --- a/src/lib/http/request.cc +++ b/src/lib/http/request.cc @@ -169,7 +169,15 @@ HttpRequest::toString() const { bool HttpRequest::isPersistent() const { - HttpHeaderPtr conn = getHeaderSafe("connection"); + HttpHeaderPtr conn; + + try { + conn = getHeader("connection"); + + } catch (...) { + // If there is an exception, it means that the header was not found. + } + std::string conn_value; if (conn) { conn_value = conn->getLowerCaseValue(); diff --git a/src/lib/http/request_parser.h b/src/lib/http/request_parser.h index 4cd278e181..851e73f179 100644 --- a/src/lib/http/request_parser.h +++ b/src/lib/http/request_parser.h @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2016-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -14,16 +14,6 @@ namespace isc { namespace http { -/// @brief Exception thrown when an error during parsing HTTP request -/// has occurred. -/// -/// The most common errors are due to receiving malformed requests. -class HttpRequestParserError : public HttpMessageParserBaseError { -public: - HttpRequestParserError(const char* file, size_t line, const char* what) : - HttpMessageParserBaseError(file, line, what) { }; -}; - class HttpRequestParser; /// @brief Pointer to the @ref HttpRequestParser. diff --git a/src/lib/http/response_json.h b/src/lib/http/response_json.h index 5c43d0b31c..d8ed06e900 100644 --- a/src/lib/http/response_json.h +++ b/src/lib/http/response_json.h @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2016-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -54,7 +54,9 @@ public: /// @brief Completes creation of the HTTP response. /// /// This method marks the response as finalized. The JSON structure is - /// created and can be used to retrieve the parsed data. + /// created and can be used to retrieve the parsed data. If this is the + /// outbound message, it can be transmitted over the wire as the body + /// for the message is now committed. virtual void finalize(); /// @brief Reset the state of the object. diff --git a/src/lib/http/response_parser.h b/src/lib/http/response_parser.h index 074ffa4f4a..fa51caa910 100644 --- a/src/lib/http/response_parser.h +++ b/src/lib/http/response_parser.h @@ -1,4 +1,4 @@ -// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -14,16 +14,6 @@ namespace isc { namespace http { -/// @brief Exception thrown when an error during parsing HTTP response -/// has occurred. -/// -/// The most common errors are due to receiving malformed response. -class HttpResponseParserError : public HttpMessageParserBaseError { -public: - HttpResponseParserError(const char* file, size_t line, const char* what) : - HttpMessageParserBaseError(file, line, what) { }; -}; - class HttpResponseParser; /// @brief Pointer to the @ref HttpResponseParser. diff --git a/src/lib/http/tests/post_request_json_unittests.cc b/src/lib/http/tests/post_request_json_unittests.cc index e8cdc792a6..48196ee41c 100644 --- a/src/lib/http/tests/post_request_json_unittests.cc +++ b/src/lib/http/tests/post_request_json_unittests.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2016-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -184,14 +184,14 @@ TEST_F(PostHttpRequestJsonTest, clientRequest) { // Commit and validate the data. ASSERT_NO_THROW(request_.finalize()); - std::ostringstream expected_response; - expected_response << "POST /isc/org HTTP/1.0\r\n" + std::ostringstream expected_request_text; + expected_request_text << "POST /isc/org HTTP/1.0\r\n" "Content-Length: " << json->str().size() << "\r\n" "Content-Type: application/json\r\n" "\r\n" << json->str(); - EXPECT_EQ(expected_response.str(), request_.toString()); + EXPECT_EQ(expected_request_text.str(), request_.toString()); } } diff --git a/src/lib/http/tests/server_client_unittests.cc b/src/lib/http/tests/server_client_unittests.cc index ac5def3b31..8415724ec5 100644 --- a/src/lib/http/tests/server_client_unittests.cc +++ b/src/lib/http/tests/server_client_unittests.cc @@ -1217,7 +1217,6 @@ TEST_F(HttpClientTest, clientRequestTimeout) { PostHttpRequestJsonPtr request1 = createRequest("partial-response", true); HttpResponseJsonPtr response1(new HttpResponseJson()); ASSERT_NO_THROW(client.asyncSendRequest(url, request1, response1, - HttpClient::RequestTimeout(100), [this, &cb_num](const boost::system::error_code& ec, const HttpResponsePtr& response, const std::string&) { @@ -1230,7 +1229,7 @@ TEST_F(HttpClientTest, clientRequestTimeout) { EXPECT_TRUE(ec.value() == boost::asio::error::timed_out); // There should be no response returned. EXPECT_FALSE(response); - })); + }, HttpClient::RequestTimeout(100))); // Create another request after the timeout. It should be handled ok. PostHttpRequestJsonPtr request2 = createRequest("sequence", 1); diff --git a/src/lib/http/url.cc b/src/lib/http/url.cc index 4f12ab5643..94ff5015f5 100644 --- a/src/lib/http/url.cc +++ b/src/lib/http/url.cc @@ -199,6 +199,9 @@ Url::parse() { // If there is anything left in the URL, we consider it a path. if (offset2 != std::string::npos) { path_ = url_.substr(offset2); + if (path_.empty()) { + path_ = "/"; + } } valid_ = true;