Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

ostree_http_repo: Reuse curl handle. #1643

Merged
merged 1 commit into from
Apr 16, 2020
Merged

ostree_http_repo: Reuse curl handle. #1643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ Our versioning scheme is `YEAR.N` where `N` is incremented whenever a new releas

## [??? (unreleased)]

### Changed

- Improved garage-deploy object fetching performance by reusing the curl handle: [PR](https://github.com/advancedtelematic/aktualizr/pull/1643)


## [2020.5] - 2020-04-01

Expand Down
15 changes: 4 additions & 11 deletions src/sota_tools/ostree_http_repo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@

#include <boost/property_tree/ini_parser.hpp>

#include "logging/logging.h"
#include "utilities/utils.h"

namespace pt = boost::property_tree;

bool OSTreeHttpRepo::LooksValid() const {
Expand Down Expand Up @@ -38,20 +35,16 @@ OSTreeRef OSTreeHttpRepo::GetRef(const std::string &refname) const { return OSTr

bool OSTreeHttpRepo::FetchObject(const boost::filesystem::path &path) const {
CURLcode err = CURLE_OK;
CurlEasyWrapper easy_handle;
curlEasySetoptWrapper(easy_handle.get(), CURLOPT_VERBOSE, get_curlopt_verbose());
server_->InjectIntoCurl(path.string(), easy_handle.get());
curlEasySetoptWrapper(easy_handle.get(), CURLOPT_WRITEFUNCTION, &OSTreeHttpRepo::curl_handle_write);
server_->InjectIntoCurl(path.string(), easy_handle_.get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

At first glance, it looks like the auth creds/certs are set for each request while it can be set once in the curl context and then just CURLOPT_URL needs to be set for each request for an object so TCC/TLS connection is reused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose the goal here is to make sure that TCP/TLS connection is reused for each request for both the downloading and pushing objects to Treehub. I suppose libcurl should do it by default as long as "_Then reusing the same easy handle will ensure it can reuse its connections.", but I am not sure if it works if we setup the creds in the "easy handle" for each request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did wonder if I should refactor that function, and there is room for some minor optimization there, but it seems like the connection is getting reused without messing with it, so I didn't change it.

Copy link
Collaborator

@mike-sul mike-sul Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👍

boost::filesystem::create_directories((root_ / path).parent_path());
std::string filename = (root_ / path).string();
int fp = open(filename.c_str(), O_WRONLY | O_CREAT, S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH);
if (fp == -1) {
LOG_ERROR << "Failed to open file: " << filename;
return false;
}
curlEasySetoptWrapper(easy_handle.get(), CURLOPT_WRITEDATA, &fp);
curlEasySetoptWrapper(easy_handle.get(), CURLOPT_FAILONERROR, true);
err = curl_easy_perform(easy_handle.get());
curlEasySetoptWrapper(easy_handle_.get(), CURLOPT_WRITEDATA, &fp);
err = curl_easy_perform(easy_handle_.get());
close(fp);

if (err == CURLE_HTTP_RETURNED_ERROR) {
Expand All @@ -62,7 +55,7 @@ bool OSTreeHttpRepo::FetchObject(const boost::filesystem::path &path) const {
} else if (err != CURLE_OK) {
// other unexpected error
char *last_url = nullptr;
curl_easy_getinfo(easy_handle.get(), CURLINFO_EFFECTIVE_URL, &last_url);
curl_easy_getinfo(easy_handle_.get(), CURLINFO_EFFECTIVE_URL, &last_url);
LOG_ERROR << "Failed to get object:" << curl_easy_strerror(err);
if (last_url != nullptr) {
LOG_ERROR << "Url: " << last_url;
Expand Down
6 changes: 6 additions & 0 deletions src/sota_tools/ostree_http_repo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@

#include <boost/filesystem.hpp>

#include "logging/logging.h"
#include "ostree_ref.h"
#include "ostree_repo.h"
#include "treehub_server.h"
#include "utilities/utils.h"

class OSTreeHttpRepo : public OSTreeRepo {
public:
Expand All @@ -14,6 +16,9 @@ class OSTreeHttpRepo : public OSTreeRepo {
if (root_.empty()) {
root_ = root_tmp_.Path();
}
curlEasySetoptWrapper(easy_handle_.get(), CURLOPT_VERBOSE, get_curlopt_verbose());
curlEasySetoptWrapper(easy_handle_.get(), CURLOPT_WRITEFUNCTION, &OSTreeHttpRepo::curl_handle_write);
curlEasySetoptWrapper(easy_handle_.get(), CURLOPT_FAILONERROR, true);
}
~OSTreeHttpRepo() override = default;

Expand All @@ -28,6 +33,7 @@ class OSTreeHttpRepo : public OSTreeRepo {
TreehubServer* server_;
boost::filesystem::path root_;
const TemporaryDirectory root_tmp_;
mutable CurlEasyWrapper easy_handle_;
};

// vim: set tabstop=2 shiftwidth=2 expandtab:
Expand Down