]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[5451] Addressed review comments.
authorMarcin Siodelski <marcin@isc.org>
Thu, 18 Jan 2018 17:24:51 +0000 (18:24 +0100)
committerMarcin Siodelski <marcin@isc.org>
Thu, 18 Jan 2018 17:24:51 +0000 (18:24 +0100)
14 files changed:
src/bin/agent/ca_response_creator.h
src/lib/http/client.cc
src/lib/http/client.h
src/lib/http/http_message.cc
src/lib/http/http_message.h
src/lib/http/http_message_parser_base.cc
src/lib/http/http_message_parser_base.h
src/lib/http/request.cc
src/lib/http/request_parser.h
src/lib/http/response_json.h
src/lib/http/response_parser.h
src/lib/http/tests/post_request_json_unittests.cc
src/lib/http/tests/server_client_unittests.cc
src/lib/http/url.cc

index 247d21c456e15a222c7cd64ed34dfea0b14e4516..d8f1dd8ae70a35a534cd6ce93f6172861ccfe67f 100644 (file)
@@ -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
index 2691735312e42239aedc7185a31022163c3891cd..7da68df0293a583e2ccd742120f201ab4e91db41 100644 (file)
@@ -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<void(boost::system::error_code ec, size_t length)>
 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");
     }
index 943b19defb4825f3164672de13b4611e9c5b943a..a25d07a5524e0857651c53932a58425d1d8d0c80 100644 (file)
@@ -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();
index fd4e4bb9b265f35bb11a2a6a2754f4e0d40d1a45..0f7166dfedd79e0b4326b1d8db56ff5affbe481c 100644 (file)
@@ -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
index 61dfb9980218d166005a8cd5f143e518072bea04..3634c80fa75181e8df937bfd433df3060e7c998f 100644 (file)
@@ -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.
index a7c01bf85fba9acb8dd7d1d779da72b3a8e63767..a5d84c660e0da89e3109a3df5f094e9becc282ad 100644 (file)
@@ -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<int>(event)));
 }
 
index e909e8ff64e26269e0dad6d4723864189612662a..78e76402133882e0993002b0fefae7f85688fdfb 100644 (file)
@@ -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) { };
 };
 
index 65847f8343bd7469ecca7b9fd8ff5768a8cc18d3..678f8b605d3dbc730273efcf6620a3dd3f909222 100644 (file)
@@ -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();
index 4cd278e18160cebf322c585afc1e4f151ad04d30..851e73f1793de9adafb04338a2a54daf468552f9 100644 (file)
@@ -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
 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.
index 5c43d0b31c6cf76a55fe44ef65d5ee9bc7cae749..d8ed06e900386669179d009b5400b15e091d331c 100644 (file)
@@ -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.
index 074ffa4f4af1d7ef947891135d141a131cc941c2..fa51caa9103c8a41992616c0fd094c602832b06f 100644 (file)
@@ -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
 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.
index e8cdc792a6f432efd8e72a87f17cecaa6a7efe0f..48196ee41cc799c980849f721e334e0a2393eb25 100644 (file)
@@ -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());
 }
 
 }
index ac5def3b3182cf3684697492a607662f0a107398..8415724ec5954bbb0d170e5fa4de31a8057e4fdc 100644 (file)
@@ -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);
index 4f12ab56438397fc782a7cbf57051a0e823d93f8..94ff5015f56668fb24c4a5c064f736dcbf2c6604 100644 (file)
@@ -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;