Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sending sync request through http_client #448

Merged
merged 26 commits into from
Jan 26, 2021
Merged

Sending sync request through http_client #448

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8195e58
add sync request
lalitb Dec 14, 2020
2e3031f
curl test
lalitb Dec 14, 2020
9a5ac8a
fix unique_ptr
lalitb Dec 14, 2020
088bae8
fix memory leak
lalitb Dec 14, 2020
86b0922
add sample example
lalitb Dec 14, 2020
f27b07b
fix timeout, and add timeout test
lalitb Dec 14, 2020
08a07f3
missing status code
lalitb Jan 13, 2021
7f6d292
fix operation
lalitb Jan 13, 2021
0c8b6d5
Merge branch 'master' into sync-http-request
lalitb Jan 19, 2021
3d8c763
review comments
lalitb Jan 19, 2021
a0bdc20
update comment
lalitb Jan 19, 2021
2d93638
fix http post
lalitb Jan 19, 2021
0344793
sync and http sync clients
lalitb Jan 22, 2021
da399cd
Merge branch 'master' into sync-http-request
lalitb Jan 22, 2021
c7f4bca
add missing file
lalitb Jan 22, 2021
daf3046
fix w3c trace context compile
lalitb Jan 22, 2021
a4c7fc5
bazel build
lalitb Jan 22, 2021
8800e9d
fix bazel
lalitb Jan 22, 2021
5509530
disabled timeout test
lalitb Jan 22, 2021
85c1efd
temporarily disable elastic exporter tests
lalitb Jan 22, 2021
6652aa3
fix asan error
lalitb Jan 22, 2021
0292a14
fix
lalitb Jan 26, 2021
a288fe8
Merge branch 'main' into sync-http-request
lalitb Jan 26, 2021
3d860ae
fix
lalitb Jan 26, 2021
47a14ce
Merge branch 'sync-http-request' of github.com:lalitb/opentelemetry-c…
lalitb Jan 26, 2021
d43d5ae
fix
lalitb Jan 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ class Session : public http_client::Session
is_session_active_ = true;
std::string url = host_ + std::string(http_request_->uri_);
auto callback_ptr = &callback;
curl_operation_.reset(new HttpOperation(http_request_->method_, url, callback_ptr,
http_request_->headers_, http_request_->body_, false,
http_request_->timeout_ms_));
curl_operation_.reset(new HttpOperation(
http_request_->method_, url, callback_ptr, RequestMode::Sync, http_request_->headers_,
http_request_->body_, false, http_request_->timeout_ms_));
curl_operation_->SendAsync([this, callback_ptr](HttpOperation &operation) {
if (operation.WasAborted())
{
Expand All @@ -153,6 +153,33 @@ class Session : public http_client::Session
});
}

virtual std::unique_ptr<http_client::Response> SendRequestSync(
Copy link
Contributor

@maxgolov maxgolov Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be much cleaner if you operate only on Response object rather than on SessionState. Let me elaborate - right now you have two possible states:

  • success - in this case you return Response and status code; or
  • failure - in this case you force the user to test if Response ptr is nullptr, and then inspect the SessionState parameter passed by reference.. which is not really Session state (because Session may have multiple requests), but rather this exact Request state.

So since in reality it is Request state, can you have this aggregated on Response object - making sure that you always return Response: failed or succeeded, always return it. It may be successful response or empty response, failed response, with an indication on Response object that it's a failed response. Then you avoid the nullptr entirely, you avoid checking on two separate objects, and you can just propagate the object elsewhere to the place that needs to make a decision what to do next with that failed request-response pair.

Could you please clarify - if the original intent of SessionManager was to support more than one concurrent request at a time? If so, then SessionState is a misleading name for a synchronous request resultt. Session may have more than one request, one failed, one successful, and SessionState name does not represent the actual SessionManager Session state.

Copy link
Member Author

@lalitb lalitb Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response object represents http-response containing - body, header and code. Any error during http handshake are returned in SessionState. This is to keep it consistent with how we achieve it in async requests:

EventHandler::OnResponse(Response &);
EventHandler::OnError(SessionState);
Session::SendRequest(EventHandler &); // async
unique_ptr<Request> Session::SendRequestSync(SessionState &); // sync

If it's fine, we can always return empty response in case of error, and the Response::status_code would be 0 in that case.

Session is more of logical entity, and doesn't represent multiple ongoing http requests.
SessionManager -> Multiple Sessions.
Single Session -> Single Request

http_client::SessionState &session_state) noexcept override
{
is_session_active_ = true;
std::string url = host_ + std::string(http_request_->uri_);
curl_operation_.reset(new HttpOperation(http_request_->method_, url, nullptr, RequestMode::Sync,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we set RequestMode::Sync in SendSync() or just deduce it from SendSync() and SendAsync(). Is the inconsistency between ctor and SendSync() handled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to know the actual sync mode in ctor. There are some initializations done in ctor, and the sync mode information is used to propagate the error/results to clients.

Is the inconsistency between ctor and SendSync() handled?

What kind of inconsistency we are talking about.

http_request_->headers_, http_request_->body_, false,
http_request_->timeout_ms_));
curl_operation_->SendSync();
session_state = curl_operation_->GetSessionState();
if (curl_operation_->WasAborted())
{
session_state = http_client::SessionState::Cancelled;
}
if (curl_operation_->GetResponseCode() >= CURL_LAST)
{
// we have a http response
auto response = std::unique_ptr<Response>(new Response());
response->headers_ = curl_operation_->GetResponseHeaders();
Copy link
Contributor

@maxgolov maxgolov Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you propagate Response code? If you are using int for that, then in Java it'd typically allocate -1 for failed response status code, which can then be used to determine if request was actually processed, as in connected and got some response code from server.

Copy link
Member Author

@lalitb lalitb Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we use uint16_t for status_code. If it's fine, we can always return empty response in case of error, and the Response::status_code would be 0 in that case ?

response->body_ = curl_operation_->GetResponseBody();
is_session_active_ = false;
return std::move(response);
}
is_session_active_ = false;
return nullptr;
}

virtual bool CancelSession() noexcept override
{
curl_operation_->Abort();
Expand Down
32 changes: 26 additions & 6 deletions ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ const std::chrono::milliseconds default_http_conn_timeout(5000); // ms
const std::string http_status_regexp = "HTTP\\/\\d\\.\\d (\\d+)\\ .*";
const std::string http_header_regexp = "(.*)\\: (.*)\\n*";

enum class RequestMode
{
Sync,
Async
};

struct curl_ci
{
bool operator()(const std::string &s1, const std::string &s2) const
Expand All @@ -45,20 +51,25 @@ class HttpOperation
public:
void DispatchEvent(http_client::SessionState type, std::string reason = "")
{
if (callback_ != nullptr)
if (request_mode_ == RequestMode::Async && callback_ != nullptr)
{
callback_->OnEvent(type, reason);
}
else
{
session_state_ = type;
}
}

std::atomic<bool> is_aborted_; // Set to 'true' when async callback is aborted
std::atomic<bool> is_finished_; // Set to 'true' when async callback is finished.

/**
* Create local CURL instance for url and body
*
* @param url
* @param method // HTTP Method
* @param url // HTTP URL
* @param callback
* @param request_mode // sync or async
* @param request Request Headers
* @param body Reques Body
* @param raw_response whether to parse the response
Expand All @@ -67,6 +78,7 @@ class HttpOperation
HttpOperation(http_client::Method method,
std::string url,
http_client::EventHandler *callback,
RequestMode request_mode = RequestMode::Async,
// Default empty headers and empty request body
const Headers &request_headers = Headers(),
const http_client::Body &request_body = http_client::Body(),
Expand All @@ -77,7 +89,7 @@ class HttpOperation
method_(method),
url_(url),
callback_(callback),

request_mode_(request_mode),
// Local vars
request_headers_(request_headers),
request_body_(request_body),
Expand Down Expand Up @@ -177,11 +189,10 @@ class HttpOperation
// curl_easy_setopt(curl, CURLOPT_LOCALPORT, dcf_port);

// Perform initial connect, handling the timeout if needed

curl_easy_setopt(curl_, CURLOPT_CONNECT_ONLY, 1L);
curl_easy_setopt(curl_, CURLOPT_TIMEOUT, http_conn_timeout_.count() / 1000);
DispatchEvent(http_client::SessionState::Connecting);
res_ = curl_easy_perform(curl_);

if (CURLE_OK != res_)
{
DispatchEvent(http_client::SessionState::ConnectFailed,
Expand Down Expand Up @@ -298,11 +309,18 @@ class HttpOperation
return result_;
}

void SendSync() { Send(); }

/**
* Get HTTP response code. This function returns CURL error code if HTTP response code is invalid.
*/
long GetResponseCode() { return res_; }

/**
* Get last session state.
*/
http_client::SessionState GetSessionState() { return session_state_; }

/**
* Get whether or not response was programmatically aborted
*/
Expand Down Expand Up @@ -389,6 +407,7 @@ class HttpOperation
protected:
const bool is_raw_response_; // Do not split response headers from response body
const std::chrono::milliseconds http_conn_timeout_; // Timeout for connect. Default: 5000ms
RequestMode request_mode_;

CURL *curl_; // Local curl instance
CURLcode res_; // Curl result OR HTTP status code if successful
Expand All @@ -401,6 +420,7 @@ class HttpOperation
const Headers &request_headers_;
const http_client::Body &request_body_;
struct curl_slist *headers_chunk_ = nullptr;
http_client::SessionState session_state_;

// Processed response headers and body
std::vector<uint8_t> resp_headers_;
Expand Down
15 changes: 15 additions & 0 deletions ext/include/opentelemetry/ext/http/client/http_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
/*
Usage Example

Async Request:

struct SimpleReponseHandler: public ResponseHandler {
void OnResponse(Response& res) noexcept override
{
Expand Down Expand Up @@ -37,6 +39,17 @@
session->FinishSession() // optionally in the end
...shutdown
sessionManager.FinishAllSessions()

Sync Request:

SessionMamager sessionManager;
auto session = sessionManager.createSession("localhost", 8000);
auto request = session->CreateRequest();
request->AddHeader(..);
SessionState session_state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is not a session state - this is given request state?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

auto response = session->SendRequestSync(session_state);
// session_state will contain SessionState::Response if successful.

*/

OPENTELEMETRY_BEGIN_NAMESPACE
Expand Down Expand Up @@ -138,6 +151,8 @@ class Session

virtual void SendRequest(EventHandler &) noexcept = 0;

virtual std::unique_ptr<Response> SendRequestSync(SessionState &) noexcept = 0;

virtual bool IsSessionActive() noexcept = 0;

virtual bool CancelSession() noexcept = 0;
Expand Down
42 changes: 36 additions & 6 deletions ext/test/http/curl_http_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,16 +248,46 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations)
std::multimap<std::string, std::string, curl::curl_ci> m1 = {
{"name1", "value1_1"}, {"name1", "value1_2"}, {"name2", "value3"}, {"name3", "value3"}};
curl::Headers headers = m1;
curl::HttpOperation http_operations1(http_client::Method::Head, "/get", handler, headers, body,
true);
curl::HttpOperation http_operations1(http_client::Method::Head, "/get", handler,
lalitb marked this conversation as resolved.
Show resolved Hide resolved
curl::RequestMode::Async, headers, body, true);
http_operations1.Send();

curl::HttpOperation http_operations2(http_client::Method::Get, "/get", handler, headers, body,
true);
curl::HttpOperation http_operations2(http_client::Method::Get, "/get", handler,
curl::RequestMode::Async, headers, body, true);
http_operations2.Send();

curl::HttpOperation http_operations3(http_client::Method::Get, "/get", handler, headers, body,
false);
curl::HttpOperation http_operations3(http_client::Method::Get, "/get", handler,
curl::RequestMode::Async, headers, body, false);
http_operations3.Send();
delete handler;
}

TEST_F(BasicCurlHttpTests, SendGetRequestSync)
{
received_requests_.clear();
curl::SessionManager session_manager;

auto session = session_manager.CreateSession("127.0.0.1", HTTP_PORT);
auto request = session->CreateRequest();
request->SetUri("get/");
http_client::SessionState session_state;
auto response = session->SendRequestSync(session_state);
EXPECT_EQ(response->GetStatusCode(), 200);
EXPECT_EQ(session_state, http_client::SessionState::Response);
}

TEST_F(BasicCurlHttpTests, SendGetRequestSyncTimeout)
{
received_requests_.clear();
curl::SessionManager session_manager;

auto session =
Copy link
Contributor

@maxgolov maxgolov Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I need to send 2 synchronous requests from two different threads, do I need to create two separate sessions?

Why do I have to specify session IP, port, and Uri separately - is there a reason to not having something like:

oReq.open("GET", "http://www.example.org/example.txt")

Copy link
Member Author

@lalitb lalitb Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I need to send 2 synchronous requests from two different threads, do I need to create two separate sessions?

Session is a logical entity here for maintaining session id, ssl cert ( if needed ), callback object ( for async request) etc. HTTP is stateless protocol, so it doesn't makes sense to have ongoing session, and sending HTTP Requests over that.

Why do I have to specify session IP, port, and Uri separately - is there a reason to not having something like:
oReq.open("GET", "http://www.example.org/example.txt")

Yeah, its doable as separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have done the modified sync requests as below:

  HttpClient httpClient;
  auto result = httpClient.Get(url);
  if (result){
    auto response = result.GetResponse();
  } else {
    std::cout << result.GetSessionState();
  }

session_manager.CreateSession("222.222.222.200", HTTP_PORT); // Non Existing address
auto request = session->CreateRequest();
request->SetTimeoutMs(std::chrono::milliseconds(3000));
request->SetUri("get/");
http_client::SessionState session_state;
auto response = session->SendRequestSync(session_state);
EXPECT_EQ(session_state, http_client::SessionState::ConnectFailed);
EXPECT_TRUE(response == nullptr);
}