Fix deadlock in standalone mcrouter on invalid config#455
Closed
mszabo-wikia wants to merge 1 commit into
Closed
Conversation
mszabo-wikia
force-pushed
the
fix-oss-config-deadlock
branch
from
cf9a8ae to
45b81a3
Compare
Contributor
|
@disylh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mszabo-wikia
force-pushed
the
fix-oss-config-deadlock
branch
from
45b81a3 to
2858489
Compare
After 6c2142a, standalone mcrouter now deadlocks if router the configuration was incorrect. Spotted on facebook#449, where `test_unknown_named_handles` started failing due to the mcrouter process being tested never existing. 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.
mszabo-wikia
force-pushed
the
fix-oss-config-deadlock
branch
from
2858489 to
080d8be
Compare
Contributor
Author
|
Formatting should hopefully be fixed in the latest commit (using https://github.com/facebook/folly/blob/abf77ba9340d7fb225c0a0fe6b355ca2a5652231/folly/.clang-format as a baseline) |
Contributor
|
@disylh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
facebook-github-bot
pushed a commit
to facebook/hhvm
that referenced
this pull request
Dec 13, 2024
Summary: After 6c2142acd8e69edd40eed70a93ea17ee2909287d, standalone mcrouter now deadlocks if the router configuration was incorrect. Spotted on facebook/mcrouter#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 6c2142acd8e69edd40eed70a93ea17ee2909287d 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. X-link: facebook/mcrouter#455 Reviewed By: lenar-f Differential Revision: D67069250 Pulled By: disylh fbshipit-source-id: d6edff40b9f40ee7925e94c4ee9cf985c10a98de
Contributor
Author
|
Thanks! |
Contributor
|
Thanks for fixing! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After 6c2142a, standalone mcrouter now deadlocks if the router configuration was incorrect. Spotted on #449, where
test_unknown_named_handlesstarted failing due to the mcrouter process being tested never exiting.GDB indicates a deadlock between three threads:
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.
Fix it by initializing the AsyncMcServer only after the router has been initialized.