Skip to content

Commit

Permalink
Rip out all of our cancellation logic, use sd_event_exit
Browse files Browse the repository at this point in the history
Originally, I wanted to address the following problem:

When a user callback indicates failure, calling CancelAll() means that
we end up (re)invoking a curl socket callback from a socket callback,
leading to a double free (either in sd-event or curl). One possible
backtrace looks like:

    (gdb) bt
->  #0  aur::AurImpl::DispatchSocketCallback (this=0x618000000480, s=<optimized out>, action=4, io=<optimized out>) at ../src/aur/aur.cc:330
    #1  0x00007ffff74fa4e1 in singlesocket () from /usr/lib/libcurl.so.4
    #2  0x00007ffff74fe622 in curl_multi_remove_handle () from /usr/lib/libcurl.so.4
    #3  0x000055555570e833 in aur::AurImpl::FinishRequest (this=<optimized out>, curl=0x623000005500, result=<optimized out>, dispatch_callback=<optimized out>) at ../src/aur/aur.cc:462
    #4  0x0000555555708cc1 in std::__do_visit<std::__detail::__variant::__deduce_visit_result<void>, aur::AurImpl::Cancel(const value_type&)::Visitor, const std::variant<void*, sd_event_source*>&> (__visitor=...) at /usr/include/c++/10.2.0/variant:869
    #5  std::visit<aur::AurImpl::Cancel(const value_type&)::Visitor, const std::variant<void*, sd_event_source*>&> (__visitor=...) at /usr/include/c++/10.2.0/variant:1710
    #6  aur::AurImpl::Cancel (this=0x618000000480, request=...) at ../src/aur/aur.cc:291
    #7  0x00005555557090f6 in aur::AurImpl::CancelAll (this=0x618000000480) at ../subprojects/abseil-cpp-20200225.2/absl/container/internal/raw_hash_set.h:311
    #8  0x000055555570f08d in aur::AurImpl::CheckFinished (this=0x618000000480) at ../src/aur/aur.cc:485
->  #9  0x000055555570f341 in aur::AurImpl::DispatchSocketCallback (this=0x618000000480, s=<optimized out>, action=4, io=<optimized out>) at ../src/aur/aur.cc:332
    #10 0x00007ffff74fb092 in Curl_multi_closed () from /usr/lib/libcurl.so.4
    #11 0x00007ffff74cc631 in Curl_closesocket () from /usr/lib/libcurl.so.4
    #12 0x00007ffff74df551 in Curl_disconnect () from /usr/lib/libcurl.so.4
    #13 0x00007ffff74fc354 in multi_done () from /usr/lib/libcurl.so.4
    #14 0x00007ffff74fcc91 in multi_runsingle () from /usr/lib/libcurl.so.4
    #15 0x00007ffff74fe1d1 in multi_socket () from /usr/lib/libcurl.so.4
    #16 0x00007ffff74fe354 in curl_multi_socket_action () from /usr/lib/libcurl.so.4
    #17 0x000055555570f60d in aur::AurImpl::OnCurlTimer (userdata=0x618000000480) at ../src/aur/aur.cc:401
    #18 0x00007ffff7466b3e in ?? () from /usr/lib/libsystemd.so.0
    #19 0x00007ffff746821e in sd_event_dispatch () from /usr/lib/libsystemd.so.0
    #20 0x00007ffff746a6a9 in sd_event_run () from /usr/lib/libsystemd.so.0
    #21 0x0000555555704bd3 in aur::AurImpl::Wait (this=0x618000000480) at ../src/aur/aur.cc:495
    #22 0x000055555563ee8a in auracle::Auracle::GetOutdatedPackages (this=<optimized out>, args=std::vector of length 0, capacity 0, packages=<optimized out>) at /usr/include/c++/10.2.0/bits/unique_ptr.h:421
    #23 0x0000555555656816 in auracle::Auracle::Outdated (this=<optimized out>, args=..., options=...) at ../src/auracle/auracle.cc:564
    #24 0x0000555555596c7f in main (argc=<optimized out>, argv=<optimized out>) at ../subprojects/abseil-cpp-20200225.2/absl/container/internal/raw_hash_set.h:311

We could do that by making CancelAll() merely schedule another event
that performs the actual cancellation at a later point in order to avoid
the recursion. However, our cancellation logic is all sorts of weird and
makes assumptions about how events are dispatched (i.e. there might be
multiple at a time). Let's just get rid of all of this and use the
sd-event mechanism of sd_event_exit instead.

This does, however (as did the original proposed solution), have the
side effect of logging multiple times because we potentially open up to
5 connections to the AUR at once, e.g.

  $ build/auracle --baseurl http://129.168.255.1 outdated
  error: UNKNOWN: Connection timed out after 10000 milliseconds
  error: UNKNOWN: Connection timed out after 10000 milliseconds

I suppose one way to fix this would be to do response merging on the
backend to match the request splitting. That way, the frontend only gets
one response. I think that comes with a lot of weird potential behaviors
though (handling of partial failures, to mention one). Would be nicer if
the AUR didn't have the crap behavior and could take POST requests in
order to extend the arg limit.

Whatever, this is a weird edge case.

Fixes #82.
  • Loading branch information
falconindy committed Sep 4, 2020
1 parent f338d45 commit 9e8b12c
Showing 1 changed file with 21 additions and 51 deletions.
72 changes: 21 additions & 51 deletions src/aur/aur.cc
Original file line number Diff line number Diff line change
@@ -40,8 +40,8 @@ class AurImpl : public Aur {
void QueueCloneRequest(const CloneRequest& request,
const CloneResponseCallback& callback) override;

// Wait for all pending requests to complete. Returns non-zero if any request
// failed or was cancelled by a callback.
// Wait for all pending requests to complete. Returns non-zero if the event
// loop fails or is terminated by a user callback.
int Wait() override;

private:
@@ -53,12 +53,10 @@ class AurImpl : public Aur {
const HttpRequest& request,
const typename ResponseHandlerType::CallbackType& callback);

int FinishRequest(CURL* curl, CURLcode result, bool dispatch_callback);
int FinishRequest(CURL* curl, CURLcode result);
int FinishRequest(sd_event_source* source);

int CheckFinished();
void CancelAll();
void Cancel(const ActiveRequests::value_type& request);

enum class DebugLevel {
// No debugging.
@@ -92,7 +90,6 @@ class AurImpl : public Aur {
sigset_t saved_ss_{};
sd_event* event_ = nullptr;
sd_event_source* timer_ = nullptr;
bool cancelled_ = false;

DebugLevel debug_level_ = DebugLevel::NONE;
std::ofstream debug_stream_;
@@ -274,31 +271,6 @@ AurImpl::~AurImpl() {
}
}

void AurImpl::Cancel(const ActiveRequests::value_type& request) {
struct Visitor {
constexpr explicit Visitor(AurImpl* aur) : aur(aur) {}

void operator()(CURL* curl) {
aur->FinishRequest(curl, CURLE_ABORTED_BY_CALLBACK,
/*dispatch_callback=*/false);
}

void operator()(sd_event_source* source) { aur->FinishRequest(source); }

AurImpl* aur;
};

std::visit(Visitor(this), request);
}

void AurImpl::CancelAll() {
while (!active_requests_.empty()) {
Cancel(*active_requests_.begin());
}

cancelled_ = true;
}

// static
int AurImpl::SocketCallback(CURLM*, curl_socket_t s, int action, void* userdata,
void* sockptr) {
@@ -401,8 +373,9 @@ int AurImpl::DispatchTimerCallback(long timeout_ms) {
return 0;
}

uint64_t usec =
absl::ToUnixMicros(absl::Now() + absl::Milliseconds(timeout_ms));
uint64_t usec;
sd_event_now(event_, CLOCK_MONOTONIC, &usec);
usec += timeout_ms * 1000;

if (timer_ != nullptr) {
if (sd_event_source_set_time(timer_, usec) < 0) {
@@ -413,7 +386,8 @@ int AurImpl::DispatchTimerCallback(long timeout_ms) {
return -1;
}
} else {
if (sd_event_add_time(event_, &timer_, CLOCK_REALTIME, usec, 0,
// TODO: use sd_event_add_time_relative once its available.
if (sd_event_add_time(event_, &timer_, CLOCK_MONOTONIC, usec, 0,
&AurImpl::OnCurlTimer, this) < 0) {
return -1;
}
@@ -422,21 +396,15 @@ int AurImpl::DispatchTimerCallback(long timeout_ms) {
return 0;
}

int AurImpl::FinishRequest(CURL* curl, CURLcode result,
bool dispatch_callback) {
int AurImpl::FinishRequest(CURL* curl, CURLcode result) {
ResponseHandler* handler;
curl_easy_getinfo(curl, CURLINFO_PRIVATE, &handler);

int r = 0;
if (dispatch_callback) {
absl::Status status =
result == CURLE_OK ? StatusFromCurlHandle(curl)
: absl::UnknownError(handler->error_buffer.data());
absl::Status status = result == CURLE_OK
? StatusFromCurlHandle(curl)
: absl::UnknownError(handler->error_buffer.data());

r = handler->RunCallback(std::move(status));
} else {
delete handler;
}
int r = handler->RunCallback(std::move(status));

active_requests_.erase(curl);
curl_multi_remove_handle(curl_multi_, curl);
@@ -459,25 +427,27 @@ int AurImpl::CheckFinished() {
return 0;
}

auto r = FinishRequest(msg->easy_handle, msg->data.result,
/* dispatch_callback = */ true);
int r = FinishRequest(msg->easy_handle, msg->data.result);
if (r < 0) {
CancelAll();
sd_event_exit(event_, r);
}

return r;
}

int AurImpl::Wait() {
cancelled_ = false;

while (!active_requests_.empty()) {
if (sd_event_run(event_, 1) < 0) {
return -EIO;
}
}

return cancelled_ ? -ECANCELED : 0;
int r;
if (sd_event_get_exit_code(event_, &r) == 0) {
return r;
}

return 0;
}

template <typename ResponseHandlerType>

0 comments on commit 9e8b12c

Please sign in to comment.