* [tarantool-patches] [PATCH 1/2] fiber: remove fiber local storage
2018-06-09 17:49 [tarantool-patches] [PATCH 0/2] Box.session.sync Vladislav Shpilevoy
@ 2018-06-09 17:49 ` Vladislav Shpilevoy
2018-06-13 21:11 ` [tarantool-patches] " Konstantin Osipov
2018-06-09 17:49 ` [tarantool-patches] [PATCH 2/2] session: fix box.session.sync() Vladislav Shpilevoy
2018-06-09 17:58 ` [tarantool-patches] Re: [PATCH 0/2] Box.session.sync Vladislav Shpilevoy
2 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-09 17:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: kostja
Replace it with more specific structures and pointers in order to
prepare to add `net` storage.
This allows to make the code working with fiber storage simpler,
remove useless wrappers and casts, and in the next patch - remove
broken session.sync and add fiber sync.
---
src/box/session.cc | 4 +--
src/box/session.h | 14 ++++------
src/box/txn.c | 2 +-
src/box/txn.h | 2 +-
src/fiber.c | 9 ++----
src/fiber.h | 80 +++++++++++++++++++----------------------------------
src/fiber_channel.c | 31 ++++++---------------
src/lua/fiber.c | 19 ++++++-------
8 files changed, 56 insertions(+), 105 deletions(-)
diff --git a/src/box/session.cc b/src/box/session.cc
index 60adc5639..f868e9ecc 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -86,10 +86,8 @@ session_on_stop(struct trigger *trigger, void * /* event */)
* after the trigger and its memory is long gone.
*/
trigger_clear(trigger);
- struct session *session = (struct session *)
- fiber_get_key(fiber(), FIBER_KEY_SESSION);
/* Destroy the session */
- session_destroy(session);
+ session_destroy(fiber_get_session(fiber()));
}
struct session *
diff --git a/src/box/session.h b/src/box/session.h
index f4e9b4109..17190ab47 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -154,7 +154,7 @@ extern struct rlist session_on_auth;
static inline struct session *
fiber_get_session(struct fiber *fiber)
{
- return (struct session *) fiber_get_key(fiber, FIBER_KEY_SESSION);
+ return fiber->storage.session;
}
/**
@@ -165,13 +165,13 @@ fiber_get_session(struct fiber *fiber)
static inline void
fiber_set_user(struct fiber *fiber, struct credentials *cr)
{
- fiber_set_key(fiber, FIBER_KEY_USER, cr);
+ fiber->storage.credentials = cr;
}
static inline void
fiber_set_session(struct fiber *fiber, struct session *session)
{
- fiber_set_key(fiber, FIBER_KEY_SESSION, session);
+ fiber->storage.session = session;
}
static inline void
@@ -227,13 +227,11 @@ current_session()
static inline struct credentials *
effective_user()
{
- struct credentials *u =
- (struct credentials *) fiber_get_key(fiber(),
- FIBER_KEY_USER);
+ struct fiber *f = fiber();
+ struct credentials *u = f->storage.credentials;
if (u == NULL) {
session_create_on_demand();
- u = (struct credentials *) fiber_get_key(fiber(),
- FIBER_KEY_USER);
+ u = f->storage.credentials;
}
return u;
}
diff --git a/src/box/txn.c b/src/box/txn.c
index e25c0e0e0..5714d485d 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -40,7 +40,7 @@ double too_long_threshold;
static inline void
fiber_set_txn(struct fiber *fiber, struct txn *txn)
{
- fiber_set_key(fiber, FIBER_KEY_TXN, (void *) txn);
+ fiber->storage.txn = txn;
}
static int
diff --git a/src/box/txn.h b/src/box/txn.h
index 4db74dfca..2bec73ec1 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -142,7 +142,7 @@ struct txn {
static inline struct txn *
in_txn()
{
- return (struct txn *) fiber_get_key(fiber(), FIBER_KEY_TXN);
+ return fiber()->storage.txn;
}
/**
diff --git a/src/fiber.c b/src/fiber.c
index 9eb7aed50..75a4846d4 100644
--- a/src/fiber.c
+++ b/src/fiber.c
@@ -612,7 +612,8 @@ fiber_recycle(struct fiber *fiber)
fiber_reset(fiber);
fiber->name[0] = '\0';
fiber->f = NULL;
- memset(fiber->fls, 0, sizeof(fiber->fls));
+ fiber->wait_pad = NULL;
+ memset(&fiber->storage, 0, sizeof(fiber->storage));
unregister_fid(fiber);
fiber->fid = 0;
region_free(&fiber->gc);
@@ -682,12 +683,6 @@ fiber_set_name(struct fiber *fiber, const char *name)
snprintf(fiber->name, sizeof(fiber->name), "%s", name);
}
-extern inline void
-fiber_set_key(struct fiber *fiber, enum fiber_key key, void *value);
-
-extern inline void *
-fiber_get_key(struct fiber *fiber, enum fiber_key key);
-
static inline void *
page_align_down(void *ptr)
{
diff --git a/src/fiber.h b/src/fiber.h
index 8231bba24..b7003aeb6 100644
--- a/src/fiber.h
+++ b/src/fiber.h
@@ -92,24 +92,6 @@ enum {
FIBER_DEFAULT_FLAGS = FIBER_IS_CANCELLABLE
};
-/**
- * \brief Pre-defined key for fiber local storage
- */
-enum fiber_key {
- /** box.session */
- FIBER_KEY_SESSION = 0,
- /** Lua fiber.storage */
- FIBER_KEY_LUA_STORAGE = 1,
- /** transaction */
- FIBER_KEY_TXN = 2,
- /** User global privilege and authentication token */
- FIBER_KEY_USER = 3,
- FIBER_KEY_MSG = 4,
- /** Storage for lua stack */
- FIBER_KEY_LUA_STACK = 5,
- FIBER_KEY_MAX = 6
-};
-
/** \cond public */
/**
@@ -349,6 +331,12 @@ struct fiber_attr {
void
fiber_attr_create(struct fiber_attr *fiber_attr);
+struct session;
+struct txn;
+struct credentials;
+struct lua_State;
+struct ipc_wait_pad;
+
struct fiber {
coro_context ctx;
/** Coro stack slab. */
@@ -395,8 +383,28 @@ struct fiber {
fiber_func f;
va_list f_data;
int f_ret;
- /** Fiber local storage */
- void *fls[FIBER_KEY_MAX];
+ /** Fiber local storage. */
+ struct {
+ /**
+ * Current transaction, session and the active
+ * user credentials are shared among multiple
+ * requests and valid even out of a former.
+ */
+ struct session *session;
+ struct credentials *credentials;
+ struct txn *txn;
+ /**
+ * Fields used by a fiber created in Lua: Lua
+ * stack and the optional fiber.storage Lua
+ * reference.
+ */
+ struct {
+ struct lua_State *stack;
+ int ref;
+ } lua;
+ } storage;
+ /** An object to wait for incoming message or a reader. */
+ struct ipc_wait_pad *wait_pad;
/** Exception which caused this fiber's death. */
struct diag diag;
char name[FIBER_NAME_MAX];
@@ -577,44 +585,12 @@ fiber_find(uint32_t fid);
void
fiber_schedule_cb(ev_loop * /* loop */, ev_watcher *watcher, int revents);
-/**
- * \brief Associate \a value with \a key in fiber local storage
- * \param fiber fiber
- * \param key pre-defined key
- * \param value value to set
- */
-inline void
-fiber_set_key(struct fiber *fiber, enum fiber_key key, void *value)
-{
- assert(key < FIBER_KEY_MAX);
- fiber->fls[key] = value;
-}
-
-
static inline bool
fiber_is_dead(struct fiber *f)
{
return f->flags & FIBER_IS_DEAD;
}
-/**
- * \brief Retrieve value by \a key from fiber local storage
- * \param fiber fiber
- * \param key pre-defined key
- * \return value from from fiber local storage
- */
-inline void *
-fiber_get_key(struct fiber *fiber, enum fiber_key key)
-{
- assert(key < FIBER_KEY_MAX);
- return fiber->fls[key];
-}
-
-/**
- * Finalizer callback
- * \sa fiber_key_on_gc()
- */
-typedef void (*fiber_key_gc_cb)(enum fiber_key key, void *arg);
typedef int (*fiber_stat_cb)(struct fiber *f, void *ctx);
int
diff --git a/src/fiber_channel.c b/src/fiber_channel.c
index 99e51a47e..4bbff45c5 100644
--- a/src/fiber_channel.c
+++ b/src/fiber_channel.c
@@ -85,9 +85,7 @@ fiber_channel_has_waiter(struct fiber_channel *ch,
if (rlist_empty(&ch->waiters))
return false;
struct fiber *f = rlist_first_entry(&ch->waiters, struct fiber, state);
- struct ipc_wait_pad *pad = (struct ipc_wait_pad *)
- fiber_get_key(f, FIBER_KEY_MSG);
- return pad->status == status;
+ return f->wait_pad->status == status;
}
bool
@@ -134,14 +132,12 @@ static inline void
fiber_channel_waiter_wakeup(struct fiber *f,
enum fiber_channel_wait_status status)
{
- struct ipc_wait_pad *pad = (struct ipc_wait_pad *)
- fiber_get_key(f, FIBER_KEY_MSG);
/*
* Safe to overwrite the status without looking at it:
* whoever is touching the status, removes the fiber
* from the wait list.
*/
- pad->status = status;
+ f->wait_pad->status = status;
/*
* fiber_channel allows an asynchronous cancel. If a fiber
* is cancelled while waiting on a timeout, it is done via
@@ -317,10 +313,7 @@ fiber_channel_put_msg_timeout(struct fiber_channel *ch,
struct fiber,
state);
/* Place the message on the pad. */
- struct ipc_wait_pad *pad = (struct ipc_wait_pad *)
- fiber_get_key(f, FIBER_KEY_MSG);
-
- pad->msg = msg;
+ f->wait_pad->msg = msg;
fiber_channel_waiter_wakeup(f, FIBER_CHANNEL_WAIT_DONE);
return 0;
@@ -355,7 +348,7 @@ fiber_channel_put_msg_timeout(struct fiber_channel *ch,
struct ipc_wait_pad pad;
pad.status = FIBER_CHANNEL_WAIT_WRITER;
pad.msg = msg;
- fiber_set_key(f, FIBER_KEY_MSG, &pad);
+ f->wait_pad = &pad;
if (first_try) {
rlist_add_tail_entry(&ch->waiters, f, state);
@@ -370,7 +363,7 @@ fiber_channel_put_msg_timeout(struct fiber_channel *ch,
* rlist_del_entry() is a no-op if already done.
*/
rlist_del_entry(f, state);
- fiber_set_key(f, FIBER_KEY_MSG, NULL);
+ f->wait_pad = NULL;
if (pad.status == FIBER_CHANNEL_WAIT_CLOSED) {
/*
@@ -423,10 +416,7 @@ fiber_channel_get_msg_timeout(struct fiber_channel *ch,
f = rlist_first_entry(&ch->waiters,
struct fiber,
state);
- struct ipc_wait_pad *pad =
- (struct ipc_wait_pad *)
- fiber_get_key(f, FIBER_KEY_MSG);
- fiber_channel_buffer_push(ch, pad->msg);
+ fiber_channel_buffer_push(ch, f->wait_pad->msg);
fiber_channel_waiter_wakeup(f,
FIBER_CHANNEL_WAIT_DONE);
}
@@ -445,10 +435,7 @@ fiber_channel_get_msg_timeout(struct fiber_channel *ch,
f = rlist_first_entry(&ch->waiters,
struct fiber,
state);
- struct ipc_wait_pad *pad =
- (struct ipc_wait_pad *)
- fiber_get_key(f, FIBER_KEY_MSG);
- *msg = pad->msg;
+ *msg = f->wait_pad->msg;
fiber_channel_waiter_wakeup(f, FIBER_CHANNEL_WAIT_DONE);
return 0;
}
@@ -461,7 +448,7 @@ fiber_channel_get_msg_timeout(struct fiber_channel *ch,
*/
struct ipc_wait_pad pad;
pad.status = FIBER_CHANNEL_WAIT_READER;
- fiber_set_key(f, FIBER_KEY_MSG, &pad);
+ f->wait_pad = &pad;
if (first_try) {
rlist_add_tail_entry(&ch->waiters, f, state);
first_try = false;
@@ -475,7 +462,7 @@ fiber_channel_get_msg_timeout(struct fiber_channel *ch,
* rlist_del_entry() is a no-op if already done.
*/
rlist_del_entry(f, state);
- fiber_set_key(f, FIBER_KEY_MSG, NULL);
+ f->wait_pad = NULL;
if (pad.status == FIBER_CHANNEL_WAIT_CLOSED) {
diag_set(ChannelIsClosed);
return -1;
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 312edd3d5..147add89b 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -294,15 +294,14 @@ static int
lua_fiber_run_f(MAYBE_UNUSED va_list ap)
{
int result;
- struct lua_State *L = (struct lua_State *)
- fiber_get_key(fiber(), FIBER_KEY_LUA_STACK);
+ struct fiber *f = fiber();
+ struct lua_State *L = f->storage.lua.stack;
int coro_ref = lua_tointeger(L, -1);
lua_pop(L, 1);
result = luaT_call(L, lua_gettop(L) - 1, LUA_MULTRET);
/* Destroy local storage */
- int storage_ref = (int)(intptr_t)
- fiber_get_key(fiber(), FIBER_KEY_LUA_STORAGE);
+ int storage_ref = f->storage.lua.ref;
if (storage_ref > 0)
luaL_unref(L, LUA_REGISTRYINDEX, storage_ref);
/*
@@ -310,7 +309,7 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap)
* We can unref child stack here,
* otherwise we have to unref child stack in join
*/
- if (fiber()->flags & FIBER_IS_JOINABLE)
+ if (f->flags & FIBER_IS_JOINABLE)
lua_pushinteger(L, coro_ref);
else
luaL_unref(L, LUA_REGISTRYINDEX, coro_ref);
@@ -343,7 +342,7 @@ fiber_create(struct lua_State *L)
* At that time we can pop coro_ref from stack
*/
lua_pushinteger(child_L, coro_ref);
- fiber_set_key(f, FIBER_KEY_LUA_STACK, child_L);
+ f->storage.lua.stack = child_L;
return f;
}
@@ -467,13 +466,11 @@ static int
lbox_fiber_storage(struct lua_State *L)
{
struct fiber *f = lbox_checkfiber(L, 1);
- int storage_ref = (int)(intptr_t)
- fiber_get_key(f, FIBER_KEY_LUA_STORAGE);
+ int storage_ref = f->storage.lua.ref;
if (storage_ref <= 0) {
lua_newtable(L); /* create local storage on demand */
storage_ref = luaL_ref(L, LUA_REGISTRYINDEX);
- fiber_set_key(f, FIBER_KEY_LUA_STORAGE,
- (void *)(intptr_t) storage_ref);
+ f->storage.lua.ref = storage_ref;
}
lua_rawgeti(L, LUA_REGISTRYINDEX, storage_ref);
return 1;
@@ -613,7 +610,7 @@ static int
lbox_fiber_join(struct lua_State *L)
{
struct fiber *fiber = lbox_checkfiber(L, 1);
- struct lua_State *child_L = fiber_get_key(fiber, FIBER_KEY_LUA_STACK);
+ struct lua_State *child_L = fiber->storage.lua.stack;
fiber_join(fiber);
struct error *e = NULL;
int num_ret = 0;
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 7+ messages in thread* [tarantool-patches] [PATCH 2/2] session: fix box.session.sync()
2018-06-09 17:49 [tarantool-patches] [PATCH 0/2] Box.session.sync Vladislav Shpilevoy
2018-06-09 17:49 ` [tarantool-patches] [PATCH 1/2] fiber: remove fiber local storage Vladislav Shpilevoy
@ 2018-06-09 17:49 ` Vladislav Shpilevoy
2018-06-13 21:13 ` [tarantool-patches] " Konstantin Osipov
2018-06-09 17:58 ` [tarantool-patches] Re: [PATCH 0/2] Box.session.sync Vladislav Shpilevoy
2 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-09 17:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: kostja
Before the patch box.session.sync() is global for the session and
is updated on each new iproto request. When the connection is
multiplexed, box.session.sync() can be changed with no finishing
a current request, if a new one arrives.
The patch makes box.session.push() local for the request,
protecting it from multiplexing mess. Box.session.sync() after
the patch can be safely used inside a request.
Closes #3450
@TarantoolBot document
Title: box.session.sync() became request local
Box.session.sync() was global for a session, so it was unusable
when the connection behind the session is multiplexed. Now
box.session.sync() is request local and can be safely used inside
the request processor.
---
src/box/iproto.cc | 11 +++++++----
src/box/session.h | 17 ++++++-----------
src/fiber.h | 27 ++++++++++++++++++---------
test/box-tap/session.test.lua | 29 ++++++++++++++++++++++++++++-
4 files changed, 59 insertions(+), 25 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 0102aec4b..759945278 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1171,7 +1171,8 @@ error:
static void
tx_fiber_init(struct session *session, uint64_t sync)
{
- session->meta.sync = sync;
+ struct fiber *f = fiber();
+ f->storage.net.sync = sync;
/*
* We do not cleanup fiber keys at the end of each request.
* This does not lead to privilege escalation as long as
@@ -1182,8 +1183,8 @@ tx_fiber_init(struct session *session, uint64_t sync)
* background tasks clean up their session in on_stop
* trigger as well.
*/
- fiber_set_session(fiber(), session);
- fiber_set_user(fiber(), &session->credentials);
+ fiber_set_session(f, session);
+ fiber_set_user(f, &session->credentials);
}
/**
@@ -1818,7 +1819,9 @@ iproto_session_fd(struct session *session)
int64_t
iproto_session_sync(struct session *session)
{
- return session->meta.sync;
+ (void) session;
+ assert(session == fiber()->storage.session);
+ return fiber()->storage.net.sync;
}
/** {{{ IPROTO_PUSH implementation. */
diff --git a/src/box/session.h b/src/box/session.h
index 17190ab47..6ed97dc46 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -66,16 +66,11 @@ extern const char *session_type_strs[];
* types, and allows to do not store attributes in struct session,
* that are used only by a session of particular type.
*/
-struct session_meta {
- union {
- /** IProto connection meta. */
- struct {
- uint64_t sync;
- void *connection;
- };
- /** Only by console is used. */
- int fd;
- };
+union session_meta {
+ /** IProto connection. */
+ void *connection;
+ /** Console file/socket descriptor. */
+ int fd;
};
/**
@@ -92,7 +87,7 @@ struct session {
uint64_t id;
enum session_type type;
/** Session metadata. */
- struct session_meta meta;
+ union session_meta meta;
/** Session user id and global grants */
struct credentials credentials;
/** Trigger for fiber on_stop to cleanup created on-demand session */
diff --git a/src/fiber.h b/src/fiber.h
index b7003aeb6..021929822 100644
--- a/src/fiber.h
+++ b/src/fiber.h
@@ -393,15 +393,24 @@ struct fiber {
struct session *session;
struct credentials *credentials;
struct txn *txn;
- /**
- * Fields used by a fiber created in Lua: Lua
- * stack and the optional fiber.storage Lua
- * reference.
- */
- struct {
- struct lua_State *stack;
- int ref;
- } lua;
+ union {
+ /**
+ * Fields used by a fiber created in Lua:
+ * Lua stack and the optional
+ * fiber.storage Lua reference.
+ */
+ struct {
+ struct lua_State *stack;
+ int ref;
+ } lua;
+ /**
+ * Fields used by a fiber created to
+ * process an iproto request.
+ */
+ struct {
+ uint64_t sync;
+ } net;
+ };
} storage;
/** An object to wait for incoming message or a reader. */
struct ipc_wait_pad *wait_pad;
diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
index 6fddced3c..c3c07a67c 100755
--- a/test/box-tap/session.test.lua
+++ b/test/box-tap/session.test.lua
@@ -15,7 +15,7 @@ session = box.session
space = box.schema.space.create('tweedledum')
index = space:create_index('primary', { type = 'hash' })
-test:plan(53)
+test:plan(55)
---
--- Check that Tarantool creates ADMIN session for #! script
@@ -183,6 +183,33 @@ local sp = conn:eval("return box.space._space.index.name:get{\"sp1\"}[2]")
test:is(sp, 1, "effective ddl owner")
conn:close()
+--
+-- gh-3450: box.session.sync() becomes request local.
+--
+cond = fiber.cond()
+local sync1, sync2
+local started = 0
+function f1()
+ started = started + 1
+ cond:wait()
+ sync1 = box.session.sync()
+end
+function f2()
+ started = started + 1
+ sync2 = box.session.sync()
+ cond:signal()
+end
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+conn = net.box.connect(box.cfg.listen)
+test:ok(conn:ping(), 'connect to self')
+_ = fiber.create(function() conn:call('f1') end)
+while started ~= 1 do fiber.sleep(0.01) end
+_ = fiber.create(function() conn:call('f2') end)
+while started ~= 2 do fiber.sleep(0.01) end
+test:isnt(sync1, sync2, 'session.sync() is request local')
+conn:close()
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
inspector:cmd('stop server session with cleanup=1')
session = nil
os.exit(test:check() == true and 0 or -1)
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 7+ messages in thread