Skip to content

Commit 9f0ceed

Browse files
Keivaun Waughfacebook-github-bot
Keivaun Waugh
authored andcommitted
Switch to VirtualEventBase
Summary: There's discussion on [this post](https://fb.prod.workplace.com/groups/560979627394613/permalink/2878701882289031/) where it was suggested to not use `runOnDestructionStart` since it special cases revproxy. Ideally that function would be removed since there's already a way to run some code on destruction start via a `VirtualEventBase`. This diff moves us to do the above. Our shutdown logic is very confusing, but at a high level what we're trying to achieve is the following: - On first SIGTERM, begin shutting down the workers. Have them continue looping until either: - All events have been processed - We've been force stopped via a second SIGTERM - After all events have been process or we've been force stopped, let the main thread know. Once all workers have notified the main thread, we shut down the main thread. Prior to these changes, the flow was: - schedule a `cleanup` callback to `runOnDestructionStart` - `cleanup` would `loop()` and also check if `forceStopped_` was set to true - the worker would then call `callback_.workerFinished(this);`, which would notify the main thread that it was done processing events Now the flow is as follows: - we manage the event bases with a virtual event bases. This also will let us schedule a callback that will be run when destruction begins. However, **we cannot** call `loop` from the callback of a virtual event base, since the virtual event base loop is driven by the main event base, and you cannot call `loop` from an already-looping callback - To address the above, we set `isWaitForAll(true)` on the executor, which will make each event base `loop()` to flush pending events before it exits (exactly as we want). Reviewed By: HsiehYuho Differential Revision: D55028184 fbshipit-source-id: d7c19269cdcb6cadf3c09414b337928ac3c3810b
1 parent a6364af commit 9f0ceed

File tree

2 files changed

+0
-7
lines changed

2 files changed

+0
-7
lines changed

proxygen/lib/services/RequestWorkerThread.cpp

-5
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,11 @@ void RequestWorkerThread::setup() {
7575
}
7676

7777
void RequestWorkerThread::forceStop() {
78-
forceStopped_.store(true);
7978
evb_->terminateLoopSoon();
8079
}
8180

8281
void RequestWorkerThread::cleanup() {
8382
LOG(INFO) << "Worker " << unsigned(getWorkerId()) << " in cleanup";
84-
if (!forceStopped_.load()) {
85-
LOG(INFO) << "Looping to finish pending work";
86-
evb_->loop();
87-
}
8883
callback_.workerFinished(this);
8984
}
9085

proxygen/lib/services/RequestWorkerThread.h

-2
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,6 @@ class RequestWorkerThread {
125125
folly::EventBase* evb_{nullptr};
126126

127127
static thread_local RequestWorkerThread* currentRequestWorker_;
128-
129-
std::atomic_bool forceStopped_{false};
130128
};
131129

132130
} // namespace proxygen

0 commit comments

Comments
 (0)