Remove request id from HTTP client This is not used for anything but debug logging BUG:24267885 Change-Id: Ia17a6f4288c8e2430db502bcc7efe5e392961990 Reviewed-on: https://weave-review.googlesource.com/1280 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/examples/ubuntu/curl_http_client.cc b/libweave/examples/ubuntu/curl_http_client.cc index 3e996e3..1b1e21f 100644 --- a/libweave/examples/ubuntu/curl_http_client.cc +++ b/libweave/examples/ubuntu/curl_http_client.cc
@@ -14,9 +14,9 @@ namespace { struct ResponseImpl : public provider::HttpClient::Response { - int GetStatusCode() const { return status; } - std::string GetContentType() const { return content_type; } - const std::string& GetData() const { return data; } + int GetStatusCode() const override { return status; } + std::string GetContentType() const override { return content_type; } + const std::string& GetData() const override { return data; } int status; std::string content_type; @@ -106,41 +106,37 @@ return std::move(response); } -int CurlHttpClient::SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) { - ++request_id_; +void CurlHttpClient::SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) { ErrorPtr error; auto response = SendRequestAndBlock(method, url, headers, data, &error); if (response) { task_runner_->PostDelayedTask( FROM_HERE, base::Bind(&CurlHttpClient::RunSuccessCallback, weak_ptr_factory_.GetWeakPtr(), success_callback, - request_id_, base::Passed(&response)), + base::Passed(&response)), {}); } else { task_runner_->PostDelayedTask( FROM_HERE, base::Bind(&CurlHttpClient::RunErrorCallback, weak_ptr_factory_.GetWeakPtr(), error_callback, - request_id_, base::Passed(&error)), + base::Passed(&error)), {}); } - return request_id_; } void CurlHttpClient::RunSuccessCallback(const SuccessCallback& success_callback, - int id, std::unique_ptr<Response> response) { - success_callback.Run(id, *response); + success_callback.Run(*response); } void CurlHttpClient::RunErrorCallback(const ErrorCallback& error_callback, - int id, ErrorPtr error) { - error_callback.Run(id, error.get()); + error_callback.Run(error.get()); } } // namespace examples
diff --git a/libweave/examples/ubuntu/curl_http_client.h b/libweave/examples/ubuntu/curl_http_client.h index 853b797..7d62b3d 100644 --- a/libweave/examples/ubuntu/curl_http_client.h +++ b/libweave/examples/ubuntu/curl_http_client.h
@@ -29,23 +29,20 @@ const Headers& headers, const std::string& data, ErrorPtr* error) override; - int SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) override; + void SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) override; private: void RunSuccessCallback(const SuccessCallback& success_callback, - int id, std::unique_ptr<Response> response); void RunErrorCallback(const ErrorCallback& error_callback, - int id, ErrorPtr error); provider::TaskRunner* task_runner_{nullptr}; - int request_id_ = 0; base::WeakPtrFactory<CurlHttpClient> weak_ptr_factory_{this}; };
diff --git a/libweave/examples/ubuntu/event_http_client.cc b/libweave/examples/ubuntu/event_http_client.cc index bdf265c..f81916c 100644 --- a/libweave/examples/ubuntu/event_http_client.cc +++ b/libweave/examples/ubuntu/event_http_client.cc
@@ -47,8 +47,8 @@ class EventHttpResponse : public weave::provider::HttpClient::Response { public: - int GetStatusCode() const { return status; } - std::string GetContentType() const { return content_type; } + int GetStatusCode() const override { return status; } + std::string GetContentType() const override { return content_type; } const std::string& GetData() const { return data; } int status; @@ -58,7 +58,6 @@ struct EventRequestState { TaskRunner* task_runner_; - int request_id_; std::unique_ptr<evhttp_uri, EventDeleter> http_uri_; std::unique_ptr<evhttp_connection, EventDeleter> evcon_; HttpClient::SuccessCallback success_callback_; @@ -75,9 +74,7 @@ "request failed: %s", evutil_socket_error_to_string(err)); state->task_runner_->PostDelayedTask( - FROM_HERE, - base::Bind(state->error_callback_, state->request_id_, error.get()), - {}); + FROM_HERE, base::Bind(state->error_callback_, error.get()), {}); return; } std::unique_ptr<EventHttpResponse> response{new EventHttpResponse()}; @@ -88,8 +85,7 @@ auto n = evbuffer_remove(buffer, &response->data[0], length); CHECK_EQ(n, int(length)); state->task_runner_->PostDelayedTask( - FROM_HERE, - base::Bind(state->success_callback_, state->request_id_, *response), {}); + FROM_HERE, base::Bind(state->success_callback_, *response), {}); } } // namespace @@ -106,14 +102,12 @@ return nullptr; } -int EventHttpClient::SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) { - ++request_id_; - +void EventHttpClient::SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) { evhttp_cmd_type method_id; CHECK(weave::StringToEnum(method, &method_id)); std::unique_ptr<evhttp_uri, EventDeleter> http_uri{ @@ -142,9 +136,8 @@ CHECK(conn); std::unique_ptr<evhttp_request, EventDeleter> req{evhttp_request_new( &RequestDoneCallback, - new EventRequestState{task_runner_, request_id_, std::move(http_uri), - std::move(conn), success_callback, - error_callback})}; + new EventRequestState{task_runner_, std::move(http_uri), std::move(conn), + success_callback, error_callback})}; CHECK(req); auto output_headers = evhttp_request_get_output_headers(req.get()); evhttp_add_header(output_headers, "Host", host); @@ -158,15 +151,13 @@ } auto res = evhttp_make_request(conn.get(), req.release(), method_id, uri.c_str()); - if (res < 0) { - ErrorPtr error; - Error::AddToPrintf(&error, FROM_HERE, "http_client", "request_failed", - "request failed: %s %s", method.c_str(), url.c_str()); - task_runner_->PostDelayedTask( - FROM_HERE, base::Bind(error_callback, request_id_, error.get()), {}); - return request_id_; - } - return request_id_; + if (res >= 0) + return; + ErrorPtr error; + Error::AddToPrintf(&error, FROM_HERE, "http_client", "request_failed", + "request failed: %s %s", method.c_str(), url.c_str()); + task_runner_->PostDelayedTask(FROM_HERE, + base::Bind(error_callback, error.get()), {}); } } // namespace examples
diff --git a/libweave/examples/ubuntu/event_http_client.h b/libweave/examples/ubuntu/event_http_client.h index 65e764e..c38ad2e 100644 --- a/libweave/examples/ubuntu/event_http_client.h +++ b/libweave/examples/ubuntu/event_http_client.h
@@ -25,16 +25,15 @@ const Headers& headers, const std::string& data, ErrorPtr* error) override; - int SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) override; + void SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) override; private: EventTaskRunner* task_runner_{nullptr}; - int request_id_ = 0; base::WeakPtrFactory<EventHttpClient> weak_ptr_factory_{this}; };
diff --git a/libweave/include/weave/provider/http_client.h b/libweave/include/weave/provider/http_client.h index 9671f0d..cfc8937 100644 --- a/libweave/include/weave/provider/http_client.h +++ b/libweave/include/weave/provider/http_client.h
@@ -28,8 +28,8 @@ }; using Headers = std::vector<std::pair<std::string, std::string>>; - using SuccessCallback = base::Callback<void(int, const Response&)>; - using ErrorCallback = base::Callback<void(int, const Error*)>; + using SuccessCallback = base::Callback<void(const Response&)>; + using ErrorCallback = base::Callback<void(const Error*)>; // TODO(vitalybuka): Remove blocking version. virtual std::unique_ptr<Response> SendRequestAndBlock( @@ -39,12 +39,12 @@ const std::string& data, ErrorPtr* error) = 0; - virtual int SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) = 0; + virtual void SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) = 0; protected: virtual ~HttpClient() = default;
diff --git a/libweave/include/weave/provider/test/mock_http_client.h b/libweave/include/weave/provider/test/mock_http_client.h index 21af699..86aca46 100644 --- a/libweave/include/weave/provider/test/mock_http_client.h +++ b/libweave/include/weave/provider/test/mock_http_client.h
@@ -40,12 +40,12 @@ const std::string& data, ErrorPtr* error) override; - int SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) override; + void SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) override; }; } // namespace test
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc index adb1994..8975c19 100644 --- a/libweave/src/device_registration_info.cc +++ b/libweave/src/device_registration_info.cc
@@ -124,10 +124,32 @@ data_, error); } - int Send(const HttpClient::SuccessCallback& success_callback, - const HttpClient::ErrorCallback& error_callback) { - return transport_->SendRequest(method_, url_, GetFullHeaders(), data_, - success_callback, error_callback); + void Send(const HttpClient::SuccessCallback& success_callback, + const HttpClient::ErrorCallback& error_callback) { + static int debug_id = 0; + ++debug_id; + VLOG(1) << "Sending request. id:" << debug_id << " method:" << method_ + << " url:" << url_; + VLOG(2) << "Request data: " << data_; + auto on_success = [](int debug_id, + const HttpClient::SuccessCallback& success_callback, + const HttpClient::Response& response) { + VLOG(1) << "Request succeeded. id:" << debug_id << " status:" << + response.GetStatusCode(); + VLOG(2) << "Response data: " << response.GetData(); + success_callback.Run(response); + }; + auto on_error = [](int debug_id, + const HttpClient::ErrorCallback& error_callback, + const Error* error) { + VLOG(1) << "Request failed, id=" << debug_id + << ", reason: " << error->GetCode() + << ", message: " << error->GetMessage(); + error_callback.Run(error); + }; + transport_->SendRequest(method_, url_, GetFullHeaders(), data_, + base::Bind(on_success, debug_id, success_callback), + base::Bind(on_error, debug_id, error_callback)); } void SetAccessToken(const std::string& access_token) { @@ -372,23 +394,20 @@ {"client_secret", GetSettings().client_secret}, {"grant_type", "refresh_token"}, }); - int request_id = sender.Send( - base::Bind(&DeviceRegistrationInfo::OnRefreshAccessTokenSuccess, - weak_factory_.GetWeakPtr(), shared_success_callback, - shared_error_callback), - base::Bind(&DeviceRegistrationInfo::OnRefreshAccessTokenError, - weak_factory_.GetWeakPtr(), shared_success_callback, - shared_error_callback)); - VLOG(1) << "Refresh access token request dispatched. Request ID = " - << request_id; + sender.Send(base::Bind(&DeviceRegistrationInfo::OnRefreshAccessTokenSuccess, + weak_factory_.GetWeakPtr(), shared_success_callback, + shared_error_callback), + base::Bind(&DeviceRegistrationInfo::OnRefreshAccessTokenError, + weak_factory_.GetWeakPtr(), shared_success_callback, + shared_error_callback)); + VLOG(1) << "Refresh access token request dispatched"; } void DeviceRegistrationInfo::OnRefreshAccessTokenSuccess( const std::shared_ptr<base::Closure>& success_callback, const std::shared_ptr<ErrorCallback>& error_callback, - int id, const HttpClient::Response& response) { - VLOG(1) << "Refresh access token request with ID " << id << " completed"; + VLOG(1) << "Refresh access token request completed"; oauth2_backoff_entry_->InformOfRequest(true); ErrorPtr error; auto json = ParseOAuthResponse(response, &error); @@ -424,9 +443,8 @@ void DeviceRegistrationInfo::OnRefreshAccessTokenError( const std::shared_ptr<base::Closure>& success_callback, const std::shared_ptr<ErrorCallback>& error_callback, - int id, const Error* error) { - VLOG(1) << "Refresh access token request with ID " << id << " failed"; + VLOG(1) << "Refresh access token failed"; oauth2_backoff_entry_->InformOfRequest(false); RefreshAccessToken(*success_callback, *error_callback); } @@ -662,8 +680,6 @@ // forget about 5xx when fetching new access token). // TODO(antonm): Add support for device removal. - VLOG(1) << "Sending cloud request '" << data->method << "' to '" << data->url - << "' with request body '" << data->body << "'"; ErrorPtr error; if (!VerifyRegistrationCredentials(&error)) { data->error_callback.Run(error.get()); @@ -684,21 +700,16 @@ RequestSender sender{data->method, data->url, http_client_}; sender.SetData(data->body, http::kJsonUtf8); sender.SetAccessToken(access_token_); - int request_id = - sender.Send(base::Bind(&DeviceRegistrationInfo::OnCloudRequestSuccess, - AsWeakPtr(), data), - base::Bind(&DeviceRegistrationInfo::OnCloudRequestError, - AsWeakPtr(), data)); - VLOG(1) << "Cloud request with ID " << request_id << " successfully sent"; + sender.Send(base::Bind(&DeviceRegistrationInfo::OnCloudRequestSuccess, + AsWeakPtr(), data), + base::Bind(&DeviceRegistrationInfo::OnCloudRequestError, + AsWeakPtr(), data)); } void DeviceRegistrationInfo::OnCloudRequestSuccess( const std::shared_ptr<const CloudRequestData>& data, - int request_id, const HttpClient::Response& response) { int status_code = response.GetStatusCode(); - VLOG(1) << "Response for cloud request with ID " << request_id - << " received with status code " << status_code; if (status_code == http::kDenied) { cloud_backoff_entry_->InformOfRequest(true); RefreshAccessToken( @@ -746,9 +757,7 @@ void DeviceRegistrationInfo::OnCloudRequestError( const std::shared_ptr<const CloudRequestData>& data, - int request_id, const Error* error) { - VLOG(1) << "Cloud request with ID " << request_id << " failed"; RetryCloudRequest(data); }
diff --git a/libweave/src/device_registration_info.h b/libweave/src/device_registration_info.h index 3a57982..74bf3a8 100644 --- a/libweave/src/device_registration_info.h +++ b/libweave/src/device_registration_info.h
@@ -155,12 +155,10 @@ void OnRefreshAccessTokenSuccess( const std::shared_ptr<base::Closure>& success_callback, const std::shared_ptr<ErrorCallback>& error_callback, - int id, const provider::HttpClient::Response& response); void OnRefreshAccessTokenError( const std::shared_ptr<base::Closure>& success_callback, const std::shared_ptr<ErrorCallback>& error_callback, - int id, const Error* error); // Parse the OAuth response, and sets registration status to @@ -195,10 +193,8 @@ void SendCloudRequest(const std::shared_ptr<const CloudRequestData>& data); void OnCloudRequestSuccess( const std::shared_ptr<const CloudRequestData>& data, - int request_id, const provider::HttpClient::Response& response); void OnCloudRequestError(const std::shared_ptr<const CloudRequestData>& data, - int request_id, const Error* error); void RetryCloudRequest(const std::shared_ptr<const CloudRequestData>& data); void OnAccessTokenRefreshed(
diff --git a/libweave/src/test/mock_http_client.cc b/libweave/src/test/mock_http_client.cc index b17645c..94cbe08 100644 --- a/libweave/src/test/mock_http_client.cc +++ b/libweave/src/test/mock_http_client.cc
@@ -21,21 +21,20 @@ MockSendRequest(method, url, headers, data, error)}; } -int MockHttpClient::SendRequest(const std::string& method, - const std::string& url, - const Headers& headers, - const std::string& data, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) { +void MockHttpClient::SendRequest(const std::string& method, + const std::string& url, + const Headers& headers, + const std::string& data, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback) { ErrorPtr error; std::unique_ptr<Response> response{ MockSendRequest(method, url, headers, data, &error)}; if (response) { - success_callback.Run(0, *response); + success_callback.Run(*response); } else { - error_callback.Run(0, error.get()); + error_callback.Run(error.get()); } - return 0; } } // namespace test