facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-07 15:27:13 +03:00
Code Issues Projects Releases Wiki Activity

Use SemiFuture for EdenMount::shutdown

Browse Source 
Summary:
The future returned by EdenMount::shutdown() can continue on an arbitrary thread. This has caused at least one deadlock in the past (D14337058).

Prevent users of EdenMount::shutdown() from continuing on the wrong thread by making shutdown return a SemiFuture instead of a Future.

This diff should not change behavior for production code, since all users of EdenMount::shutdown() already use Future<>::via().

Reviewed By: simpkins

Differential Revision: D14389607

fbshipit-source-id: 821d206d01ea3dcb0261cd0c7ca2d591b2c84e7a
This commit is contained in:
Matt Glazar 2019-03-12 19:25:55 -07:00 committed by Facebook Github Bot
parent 3b9a0310a1
commit 52e6a3a364
3 changed files with 13 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
eden/fs/inodes/EdenMount.cpp
View File

@ -450,7 +450,7 @@ void EdenMount::destroy() {
<< " in unexpected state " << oldState;
}
Future<SerializedInodeMap> EdenMount::shutdown(
folly::SemiFuture<SerializedInodeMap> EdenMount::shutdown(
bool doTakeover,
bool allowFuseNotStarted) {
// shutdown() should only be called on mounts that have not yet reached
@ -469,7 +469,7 @@ Future<SerializedInodeMap> EdenMount::shutdown(
return shutdownImpl(doTakeover);
}
Future<SerializedInodeMap> EdenMount::shutdownImpl(bool doTakeover) {
folly::SemiFuture<SerializedInodeMap> EdenMount::shutdownImpl(bool doTakeover) {
journal_.cancelAllSubscribers();
XLOG(DBG1) << "beginning shutdown for EdenMount " << getPath();

4
eden/fs/inodes/EdenMount.h
View File

@ -156,7 +156,7 @@ class EdenMount {
* If doTakeover is false, this function will return default-constructed
* SerializedFileHandleMap and SerializedInodeMap instances.
*/
folly::Future<SerializedInodeMap> shutdown(
folly::SemiFuture<SerializedInodeMap> shutdown(
bool doTakeover,
bool allowFuseNotStarted = false);
@ -590,7 +590,7 @@ class EdenMount {
folly::Future<TreeInodePtr> createRootInode(
const ParentCommits& parentCommits);
FOLLY_NODISCARD folly::Future<folly::Unit> setupDotEden(TreeInodePtr root);
folly::Future<SerializedInodeMap> shutdownImpl(bool doTakeover);
folly::SemiFuture<SerializedInodeMap> shutdownImpl(bool doTakeover);
std::unique_ptr<DiffContext> createDiffContext(
InodeDiffCallback* callback,

13
eden/fs/inodes/test/EdenMountTest.cpp
View File

@ -14,6 +14,7 @@
#include <folly/ScopeGuard.h>
#include <folly/chrono/Conv.h>
#include <folly/executors/ManualExecutor.h>
#include <folly/futures/FutureSplitter.h>
#include <folly/futures/Promise.h>
#include <folly/test/TestUtils.h>
#include <gtest/gtest.h>
@ -728,16 +729,20 @@ TEST(EdenMountState, mountIsFuseErrorAfterFuseInitializationFails) {
TEST(EdenMountState, mountIsShuttingDownWhileInodeIsReferencedDuringShutdown) {
auto testMount = TestMount{FakeTreeBuilder{}};
auto& mount = *testMount.getEdenMount();
auto* executor = testMount.getServerExecutor().get();
auto inode = mount.getInodeMap()->getRootInode();
auto shutdownFuture =
mount.shutdown(/*doTakeover=*/false, /*allowFuseNotStarted=*/true);
auto shutdownFutures = folly::FutureSplitter<SerializedInodeMap>{
mount.shutdown(/*doTakeover=*/false, /*allowFuseNotStarted=*/true)
.via(executor)};
SCOPE_EXIT {
inode.reset();
std::move(shutdownFuture).get(kTimeout);
shutdownFutures.getFuture().within(kTimeout).getVia(executor);
};
EXPECT_FALSE(shutdownFuture.wait(kMicroTimeout).isReady())
EXPECT_THROW(
shutdownFutures.getFuture().within(kMicroTimeout).getVia(executor),
folly::FutureTimeout)
<< "shutdown should not finish while inode is referenced";
EXPECT_EQ(mount.getState(), EdenMount::State::SHUTTING_DOWN);
}