Commit 34b79e43 authored by Jonathan Sailor's avatar Jonathan Sailor Committed by Facebook Github Bot

add a comment about some sketchy behavior

Summary:
`IOThreadPoolExecutor.pickThread()` has some sketchy behavior where, if the
threadpool has no threads, it returns `*thisThread_` unconditionally. My read
is that this can happen only in two cases.

First, `getEventBase` could be called from outside the threadpool while the
threadpool has no threads (either because it was stopped or because somebody
set it to 0 threads.) In this case, `*thisThread_` will be nullptr and
`getEventBase` will try to immediately dereference it, (hopefully) crashing.

Second, `getEventBase` could be called from inside the threadpool, but from a
thread which is shutting down. This is also somewhat dodgy, because by the time
we get to this case (`me && !contains(threadList_.get(), me)`), stopThreads
will have already called terminateLoopSoon on the evb. While it would still be
possible to use the evb safely (in the sense that one could write a program
which does not hit undefined behavior), I think it'd be strongly dependent on
the way IOTPE and EventBase are currently implemented and therefore probably a
bad idea.

I don't have the bandwidth to really dig into this now and figure out what the
correct behavior is, or how to fix people who might be relying on the broken
one. But I figure I can at least leave a comment behind that this is something
maybe worth looking into.

Reviewed By: meyering

Differential Revision: D9754314

fbshipit-source-id: 6851d49e634707cacc803a95763eda023967b932
parent d233d995
...@@ -118,6 +118,13 @@ IOThreadPoolExecutor::pickThread() { ...@@ -118,6 +118,13 @@ IOThreadPoolExecutor::pickThread() {
} }
auto n = ths.size(); auto n = ths.size();
if (n == 0) { if (n == 0) {
// XXX I think the only way this can happen is if somebody calls
// getEventBase (1) from one of the executor's threads while the executor
// is stopping or getting downsized to zero or (2) from outside the executor
// when it has no threads. In the first case, it's not obvious what the
// correct behavior should be-- do we really want to return ourselves even
// though we're about to exit? (The comment above seems to imply no.) In
// the second case, `!me` so we'll crash anyway.
return me; return me;
} }
auto thread = ths[nextThread_.fetch_add(1, std::memory_order_relaxed) % n]; auto thread = ths[nextThread_.fetch_add(1, std::memory_order_relaxed) % n];
......
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