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
36 changes: 28 additions & 8 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 @@ -4,6 +4,8 @@

#include <eosio/chain/exceptions.hpp>

#include <atomic>

#include <signal.h>

namespace eosio { namespace chain {
Expand All @@ -18,11 +20,22 @@ struct platform_timer {
/* 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_expiry_callback(void(*func)(void*), void* user) {
EOS_ASSERT(!(func && __atomic_load_n(&_callback_enabled, __ATOMIC_CONSUME)), misc_exception, "Setting a platform_timer callback when one already exists");
void set_expiration_callback(void(*func)(void*), void* user) {
callback_state current_state;
while((current_state = _callback_state.load(std::memory_order_acquire)) == READING) {} //wait for call_expiration_callback() to be done with it
if(func)
EOS_ASSERT(current_state == UNSET, misc_exception, "Setting a platform_timer callback when one already exists");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this assert will sometimes fire and sometimes not, such that you cannot make assumptions in the remaining code that _callback_state is eitherSET or func is nullptr as the _callback_state can change between this line and later lines where it is modified.

if(!func && current_state == UNSET)
return;

//prepare for loading in new values by moving from UNSET->WRITING
while(!atomic_compare_exchange_strong(&_callback_state, &current_state, WRITING))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble working out the control flow in here. There are ideally two expectations here: we are "clearing" the callback and the _callback_state is SET or we are "setting" the callback and _callback_state is UNSET`

After the first iteration of the loop, we change our expected _callback_state to UNSET even if we are going to clear. The code after is safe but wont this spinlock spin until something else clears the callback if it misses its first time through the loop (say if a callback fired at a bad time)?

current_state = UNSET;
_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;
__atomic_store_n(&_callback_enabled, !!_expiration_callback, __ATOMIC_RELEASE);

//finally go WRITING->SET
_callback_state.store(func ? SET : UNSET, std::memory_order_release);
}

volatile sig_atomic_t expired = 1;
Expand All @@ -32,17 +45,24 @@ struct platform_timer {
constexpr static size_t fwd_size = 8;
fc::fwd<impl,fwd_size> my;

enum callback_state {
UNSET,
READING,
WRITING,
SET
};

void call_expiration_callback() {
if(__atomic_load_n(&_callback_enabled, __ATOMIC_CONSUME)) {
callback_state state_is_SET = SET;
if(atomic_compare_exchange_strong(&_callback_state, &state_is_SET, READING)) {
void(*cb)(void*) = _expiration_callback;
void* cb_data = _expiration_callback_data;
if(__atomic_load_n(&_callback_enabled, __ATOMIC_CONSUME))
cb(cb_data);
_callback_state.store(SET, std::memory_order_release);
cb(cb_data);
}
}

bool _callback_enabled = false;
static_assert(__atomic_always_lock_free(sizeof(_callback_enabled), 0), "eosio requires an always lock free bool type");
std::atomic<callback_state> _callback_state = UNSET;
void(*_expiration_callback)(void*);
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)

Expand Down
4 changes: 2 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 @@ -18,8 +18,8 @@ namespace eosio { namespace chain {

/* 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);
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:
Expand Down
6 changes: 3 additions & 3 deletions libraries/chain/transaction_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +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);
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_expiry_callback(nullptr, nullptr);
_timer.set_expiration_callback(nullptr, nullptr);
}

transaction_context::transaction_context( controller& c,
Expand Down