Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

callback support for checktime timer expiry #7803

Merged
merged 4 commits into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 36 additions & 0 deletions libraries/chain/include/eosio/chain/platform_timer.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
#pragma once
#include <fc/time.hpp>
#include <fc/fwd.hpp>
#include <fc/scoped_exit.hpp>

#include <eosio/chain/exceptions.hpp>

#include <atomic>

#include <signal.h>

Expand All @@ -13,12 +18,43 @@ struct platform_timer {
void start(fc::time_point tp);
void stop();

/* Sets a callback for when timer expires. Be aware this could might fire from a signal handling context and/or
on any particular thread. Only a single callback can be registered at once; trying to register more will
result in an exception. Setting to nullptr disables any current set callback */
void set_expiration_callback(void(*func)(void*), void* user) {
bool expect_false = false;
while(!atomic_compare_exchange_strong(&_callback_variables_busy, &expect_false, true))
expect_false = false;
auto reset_busy = fc::make_scoped_exit([this]() {
_callback_variables_busy.store(false, std::memory_order_release);
});
EOS_ASSERT(!(func && _expiration_callback), misc_exception, "Setting a platform_timer callback when one already exists");

_expiration_callback = func;
Copy link
Contributor

@b1bart b1bart Aug 27, 2019

Choose a reason for hiding this comment

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

I'm worried that there is more than just the one "Race" that you mentioned in the PR here. Given FuncT1 DataT1 FuncT2 DataT2 (where one pair may be nullptr) Depending on how the compiler orders these instructions on both set_expiry_callback and call_expiration_callback you can at least get any of the following cases:

if (FuncT1) FuncT1(DataT1);
if (FuncT1) FuncT2(DataT1)
if (FuncT1) FuncT1(DataT2)
if (FuncT1) FuncT2(DataT2)
if (FuncT2) FuncT2(DataT1)
if (FuncT2) FuncT2(DataT2)

I think I understand in that in the current use case where T2 is practically going to be nullptr that boils down to:
if (FuncT1) FuncT1(DataT1);
if (FuncT1) nullptr(DataT1)
if (FuncT1) FuncT1(nullptr)
if (FuncT1) nullptr(nullptr)
if (nullptr)...
if (nullptr)...

so, 2 out of 6 of those are crashes, 1 out of 6 calls FuncT1 in an unexpected way. 1 out of 6 calls FuncT1 "spuriously" (the noted race in the PR) and the remaining 2 operate as expected.

We can improve this dramatically if we can guarantee that the pair is atomically set and queried. Does the signal handling context allow us to use atomic primitives like CAS?

Or can we double buffer the function+data pair with some sort of data barrier and then "swap" the active buffer with a single index update?

_expiration_callback_data = user;
}

volatile sig_atomic_t expired = 1;

private:
struct impl;
constexpr static size_t fwd_size = 8;
fc::fwd<impl,fwd_size> my;

void call_expiration_callback() {
bool expect_false = false;
if(atomic_compare_exchange_strong(&_callback_variables_busy, &expect_false, true)) {
void(*cb)(void*) = _expiration_callback;
void* cb_data = _expiration_callback_data;
_callback_variables_busy.store(false, std::memory_order_release);
if(cb)
cb(cb_data);
}
}

std::atomic_bool _callback_variables_busy = false;
void(*_expiration_callback)(void*) = nullptr;
void* _expiration_callback_data;
};
Copy link
Contributor

@heifner heifner Aug 28, 2019

Choose a reason for hiding this comment

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

I don't see a "carries a dependency" relationship here between _callback_enabled and _expiration_callback so __ATOMIC_CONSUME will not work correctly. Did you mean __ATOMIC_ACQUIRE ? With atomic acquire this should work just fine and the second __atomic_load_n is not needed.

This https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html indicates __ATOMIC_CONSUME is implemented as __ATOMIC_ACQUIRE anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/atomic/memory_order

The specification of release-consume ordering is being revised, and the use of memory_order_consume is temporarily discouraged. (since C++17)


}}
9 changes: 7 additions & 2 deletions libraries/chain/include/eosio/chain/transaction_context.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ namespace eosio { namespace chain {
void start(fc::time_point tp);
void stop();

/* Sets a callback for when timer expires. Be aware this could might fire from a signal handling context and/or
on any particular thread. Only a single callback can be registered at once; trying to register more will
result in an exception. Use nullptr to disable a previously set callback. */
void set_expiration_callback(void(*func)(void*), void* user);

volatile sig_atomic_t& expired;
private:
platform_timer& _timer;
Expand Down Expand Up @@ -125,6 +130,8 @@ namespace eosio { namespace chain {
int64_t billed_cpu_time_us = 0;
bool explicit_billed_cpu_time = false;

transaction_checktime_timer transaction_timer;

private:
bool is_initialized = false;

Expand All @@ -145,8 +152,6 @@ namespace eosio { namespace chain {
fc::time_point pseudo_start;
fc::microseconds billed_time;
fc::microseconds billing_timer_duration_limit;

transaction_checktime_timer transaction_timer;
};

} }
1 change: 1 addition & 0 deletions libraries/chain/platform_timer_asio_fallback.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ void platform_timer::start(fc::time_point tp) {
if(ec)
return;
expired = 1;
call_expiration_callback();
});
}
}
Expand Down
9 changes: 6 additions & 3 deletions libraries/chain/platform_timer_macos.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@ platform_timer::platform_timer() {
struct kevent64_s anEvent;
int c = kevent64(kqueue_fd, NULL, 0, &anEvent, 1, 0, NULL);

if(c == 1 && anEvent.filter == EVFILT_TIMER)
*(sig_atomic_t*)(anEvent.udata) = 1;
if(c == 1 && anEvent.filter == EVFILT_TIMER) {
platform_timer* self = (platform_timer*)anEvent.udata;
self->expired = 1;
self->call_expiration_callback();
}
else if(c == 1 && anEvent.filter == EVFILT_USER)
return;
else if(c == -1 && errno == EINTR)
Expand Down Expand Up @@ -86,7 +89,7 @@ void platform_timer::start(fc::time_point tp) {
expired = 1;
else {
struct kevent64_s aTimerEvent;
EV_SET64(&aTimerEvent, my->timerid, EVFILT_TIMER, EV_ADD|EV_ENABLE|EV_ONESHOT, NOTE_USECONDS|NOTE_CRITICAL, (int)x.count(), (uint64_t)&expired, 0, 0);
EV_SET64(&aTimerEvent, my->timerid, EVFILT_TIMER, EV_ADD|EV_ENABLE|EV_ONESHOT, NOTE_USECONDS|NOTE_CRITICAL, (int)x.count(), (uint64_t)this, 0, 0);

expired = 0;
if(kevent64(kqueue_fd, &aTimerEvent, 1, NULL, 0, KEVENT_FLAG_IMMEDIATE, NULL) != 0)
Expand Down
6 changes: 4 additions & 2 deletions libraries/chain/platform_timer_posix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ struct platform_timer::impl {
timer_t timerid;

static void sig_handler(int, siginfo_t* si, void*) {
*(sig_atomic_t*)(si->si_value.sival_ptr) = 1;
platform_timer* self = (platform_timer*)si->si_value.sival_ptr;
self->expired = 1;
self->call_expiration_callback();
}
};

Expand All @@ -38,7 +40,7 @@ platform_timer::platform_timer() {
struct sigevent se;
se.sigev_notify = SIGEV_SIGNAL;
se.sigev_signo = SIGRTMIN;
se.sigev_value.sival_ptr = (void*)&expired;
se.sigev_value.sival_ptr = (void*)this;

FC_ASSERT(timer_create(CLOCK_REALTIME, &se, &my->timerid) == 0, "failed to create timer");

Expand Down
7 changes: 6 additions & 1 deletion libraries/chain/transaction_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ namespace eosio { namespace chain {
_timer.stop();
}

void transaction_checktime_timer::set_expiration_callback(void(*func)(void*), void* user) {
_timer.set_expiration_callback(func, user);
}

transaction_checktime_timer::~transaction_checktime_timer() {
stop();
_timer.set_expiration_callback(nullptr, nullptr);
}

transaction_context::transaction_context( controller& c,
Expand All @@ -49,9 +54,9 @@ namespace eosio { namespace chain {
,undo_session()
,trace(std::make_shared<transaction_trace>())
,start(s)
,transaction_timer(std::move(tmr))
,net_usage(trace->net_usage)
,pseudo_start(s)
,transaction_timer(std::move(tmr))
{
if (!c.skip_db_sessions()) {
undo_session = c.mutable_db().start_undo_session(true);
Expand Down