Skip to content

Commit

Permalink
Fix deadlock in standalone mcrouter on invalid config (#455)
Browse files Browse the repository at this point in the history
Summary:
After 6c2142a, standalone mcrouter now deadlocks if the router configuration was incorrect. Spotted on #449, where `test_unknown_named_handles` started failing due to the mcrouter process being tested never exiting.

GDB [indicates](https://gist.github.com/mszabo-wikia/47916c5655deffdb95332e972a52caf8) a deadlock between three threads:
```
(gdb) info threads
  Id   Target Id                                          Frame
* 1    Thread 0x7f7b4cce1e00 (LWP 211878) "mcrouter"      syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
  2    Thread 0x7f7b43fff6c0 (LWP 211882) "mcr-cpuaux-0"  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
  3    Thread 0x7f7b49e0a6c0 (LWP 211879) "IOThreadPool0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
```

* Thread 1, the main thread, has triggered exit() and is waiting on the auxiliary thread pool to be destroyed.
* Thread 2, an auxiliary thread pool thread, is in the process of destroying the CarbonRouterInstance due to
6c2142a and is blocked on destroying the virtual EVBs by child proxies. These however are ultimately sourced from the IO thread pool which is also used by AsyncMcServer.
* Thread 3, an IO thread pool thread, is blocked by AsyncMcServer which is waiting to be started by later initialization code that never runs due to the config error, preventing the IO thread pool itself from being destroyed.

Fix it by initializing the AsyncMcServer only after the router has been initialized.

Pull Request resolved: #455

Reviewed By: lenar-f

Differential Revision: D67069250

Pulled By: disylh

fbshipit-source-id: d6edff40b9f40ee7925e94c4ee9cf985c10a98de
  • Loading branch information
mszabo-wikia authored and facebook-github-bot committed Dec 13, 2024
1 parent 9b6c365 commit 0d6f0a8
Showing 1 changed file with 9 additions and 9 deletions.
18 changes: 9 additions & 9 deletions mcrouter/Server-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -310,11 +310,6 @@ bool runServerDual(
auto evbs = extractEvbs(*ioThreadPool);
CHECK_EQ(evbs.size(), mcrouterOpts.num_proxies);

// Create AsyncMcServer instance
asyncMcServer =
std::make_shared<AsyncMcServer>(detail::createAsyncMcServerOptions(
mcrouterOpts, standaloneOpts, &evbs));

// Create CarbonRouterInstance
if (standaloneOpts.remote_thread) {
router =
Expand All @@ -328,6 +323,11 @@ bool runServerDual(
return false;
}

// Create AsyncMcServer instance
asyncMcServer =
std::make_shared<AsyncMcServer>(detail::createAsyncMcServerOptions(
mcrouterOpts, standaloneOpts, &evbs));

setupRouter<RouterInfo>(mcrouterOpts, standaloneOpts, router, preRunCb);

// Create CarbonRouterClients for each worker thread
Expand Down Expand Up @@ -515,10 +515,6 @@ bool runServer(
// Get EVB of main thread
auto localEvb = ioThreadPool->getEventBaseManager()->getEventBase();

asyncMcServer =
std::make_shared<AsyncMcServer>(detail::createAsyncMcServerOptions(
mcrouterOpts, standaloneOpts, &evbs));

if (standaloneOpts.remote_thread) {
router =
CarbonRouterInstance<RouterInfo>::init("standalone", mcrouterOpts);
Expand All @@ -531,6 +527,10 @@ bool runServer(
return false;
}

asyncMcServer =
std::make_shared<AsyncMcServer>(detail::createAsyncMcServerOptions(
mcrouterOpts, standaloneOpts, &evbs));

setupRouter<RouterInfo>(mcrouterOpts, standaloneOpts, router, preRunCb);

auto shutdownStarted = std::make_shared<std::atomic<bool>>(false);
Expand Down

0 comments on commit 0d6f0a8

Please sign in to comment.