Commit d0f956f5 authored by Misha Shneerson's avatar Misha Shneerson Committed by Facebook Github Bot

rootid should be unique

Summary:
rootid is the thing we use to correlate running Thrift requests with a folly::RequestContext. The problem is that today rootid is unique only for active requests but not unique throughout the lifetime of the process - as a consequence we see some subtle bugs like mistakenly attributing threads to already finished requests. The root cause of rootid collisions is that default implementation of rootid is just an address of folly::RequestContext - so once request finishes the address is reused.

The solution suggested here is to assign rootids explicitly when folly::RequestContext is set for the request. To construct such rootid we pack two counters:
* 12 MSB are dedicated to an ID of Cpp2Worker (should be more than enough)
* 51 LSB are dedicated to a running counter within Cpp2Worker.

(Note: this ignores all push blocking failures!)

Reviewed By: andriigrynenko

Differential Revision: D20036950

fbshipit-source-id: 76687db12a32f33c82ce86accb4cdaab3e981458
parent 54c6d875
...@@ -466,22 +466,23 @@ RequestContext::RequestContext() ...@@ -466,22 +466,23 @@ RequestContext::RequestContext()
RequestContext::RequestContext(intptr_t rootid) RequestContext::RequestContext(intptr_t rootid)
: useHazptr_(FLAGS_folly_reqctx_use_hazptr), rootId_(rootid) {} : useHazptr_(FLAGS_folly_reqctx_use_hazptr), rootId_(rootid) {}
RequestContext::RequestContext(const RequestContext& ctx, RootTag) RequestContext::RequestContext(const RequestContext& ctx, intptr_t rootid, Tag)
: RequestContext(ctx) { : RequestContext(ctx) {
rootId_ = reinterpret_cast<intptr_t>(this); rootId_ = rootid;
} }
RequestContext::RequestContext(const RequestContext& ctx, ChildTag) RequestContext::RequestContext(const RequestContext& ctx, Tag)
: RequestContext(ctx) {} : RequestContext(ctx) {}
/* static */ std::shared_ptr<RequestContext> RequestContext::copyAsRoot( /* static */ std::shared_ptr<RequestContext> RequestContext::copyAsRoot(
const RequestContext& ctx) { const RequestContext& ctx,
return std::make_shared<RequestContext>(ctx, RootTag{}); intptr_t rootid) {
return std::make_shared<RequestContext>(ctx, rootid, Tag{});
} }
/* static */ std::shared_ptr<RequestContext> RequestContext::copyAsChild( /* static */ std::shared_ptr<RequestContext> RequestContext::copyAsChild(
const RequestContext& ctx) { const RequestContext& ctx) {
return std::make_shared<RequestContext>(ctx, ChildTag{}); return std::make_shared<RequestContext>(ctx, Tag{});
} }
bool RequestContext::doSetContextDataLock( bool RequestContext::doSetContextDataLock(
......
...@@ -156,7 +156,9 @@ class RequestContext { ...@@ -156,7 +156,9 @@ class RequestContext {
RequestContext& operator=(RequestContext&&) = delete; RequestContext& operator=(RequestContext&&) = delete;
// copy ctor is disabled, use copyAsRoot/copyAsChild instead. // copy ctor is disabled, use copyAsRoot/copyAsChild instead.
static std::shared_ptr<RequestContext> copyAsRoot(const RequestContext& ctx); static std::shared_ptr<RequestContext> copyAsRoot(
const RequestContext& ctx,
intptr_t rootid);
static std::shared_ptr<RequestContext> copyAsChild(const RequestContext& ctx); static std::shared_ptr<RequestContext> copyAsChild(const RequestContext& ctx);
// Create a unique request context for this request. // Create a unique request context for this request.
...@@ -263,13 +265,12 @@ class RequestContext { ...@@ -263,13 +265,12 @@ class RequestContext {
} }
private: private:
struct ChildTag {}; struct Tag {};
struct RootTag {};
RequestContext(const RequestContext& ctx) = default; RequestContext(const RequestContext& ctx) = default;
public: public:
RequestContext(const RequestContext& ctx, RootTag tag); RequestContext(const RequestContext& ctx, intptr_t rootid, Tag tag);
RequestContext(const RequestContext& ctx, ChildTag tag); RequestContext(const RequestContext& ctx, Tag tag);
explicit RequestContext(intptr_t rootId); explicit RequestContext(intptr_t rootId);
using StaticContext = std::pair<std::shared_ptr<RequestContext>, intptr_t>; using StaticContext = std::pair<std::shared_ptr<RequestContext>, intptr_t>;
......
...@@ -359,15 +359,15 @@ TEST_F(RequestContextTest, ShallowCopyClear) { ...@@ -359,15 +359,15 @@ TEST_F(RequestContextTest, ShallowCopyClear) {
} }
TEST_F(RequestContextTest, RootIdOnCopy) { TEST_F(RequestContextTest, RootIdOnCopy) {
auto ctxBase = std::make_shared<RequestContext>(); auto ctxBase = std::make_shared<RequestContext>(0xab);
EXPECT_EQ(reinterpret_cast<intptr_t>(ctxBase.get()), ctxBase->getRootId()); EXPECT_EQ(0xab, ctxBase->getRootId());
{ {
auto ctx = RequestContext::copyAsRoot(*ctxBase); auto ctx = RequestContext::copyAsRoot(*ctxBase, 0xabc);
EXPECT_EQ(reinterpret_cast<intptr_t>(ctx.get()), ctx->getRootId()); EXPECT_EQ(0xabc, ctx->getRootId());
} }
{ {
auto ctx = RequestContext::copyAsChild(*ctxBase); auto ctx = RequestContext::copyAsChild(*ctxBase);
EXPECT_EQ(reinterpret_cast<intptr_t>(ctxBase.get()), ctx->getRootId()); EXPECT_EQ(0xab, ctx->getRootId());
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment