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

callback support for checktime timer expiry #7803

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
357e30c
callback support for the checktime timer
spoonincode Aug 20, 2019
c072a01
double check if a timer callback is active after double buffering the…
spoonincode Aug 27, 2019
f959c5a
refactor platform timer callback "thread" safety further
spoonincode Aug 28, 2019
c79c769
further simplification to timer callbacks
spoonincode Aug 29, 2019
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
19 changes: 19 additions & 0 deletions libraries/chain/include/eosio/chain/platform_timer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
#include <fc/time.hpp>
#include <fc/fwd.hpp>

#include <eosio/chain/exceptions.hpp>

#include <signal.h>

namespace eosio { namespace chain {
Expand All @@ -13,12 +15,29 @@ 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 is allowed as a "no callback" hint */
void set_expiry_callback(void(*func)(void*), void* user) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to remain consistent with the name of the member variable and the call_expiration_callback method this should probably be renamed to set_expiration_callback

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 •
edited
Loading

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() {
if(_expiration_callback)
_expiration_callback(_expiration_callback_data);
}

void(*_expiration_callback)(void*) = nullptr;
void* _expiration_callback_data;
};
Copy link
Contributor

@heifner heifner Aug 28, 2019 •
edited
Loading

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
View file
Open in desktop
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. */
void set_expiry_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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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_expiry_callback(void(*func)(void*), void* user) {
_timer.set_expiry_callback(func, user);
}

transaction_checktime_timer::~transaction_checktime_timer() {
stop();
_timer.set_expiry_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