facebook / facebook/folly

What is the expected behavior of Future::thenInline?

Open
#2,088 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
C++
Stars
30.5k
Forks
5.9k
PR merge metrics
No merged PRs in 30d

Description

Hi, team

I have this PR to change the behavior of `Future::thenInline` when the previous Promise finished early, which I think is bug #2087 , could you please take a look, we could discuss this in the PR.

Also, when the previous callback returns a Future that is actually fulfilled in another executor, what should be the expected behavior? I would expect it to be executed in line with where the promise was fulfilled, as the below test illustrated, but this case failed, and we actually scheduled the inline callback to other executors.
```
TEST(Then, DISABLED_thenInlineFollowContinuationReturnsNonImmediateFuture) {
CPUThreadPoolExecutor e1{4};
uint64_t threadId1 = 0;
folly::makeFuture()
.via(&e1)
.thenValue([&threadId1](auto&&) {
return folly::via(folly::getGlobalCPUExecutor())
.then([&threadId1](auto&&) {
threadId1 = folly::getCurrentThreadID();
return true;
});
})
.thenInline([&threadId1](auto&&) {
auto threadId2 = folly::getCurrentThreadID();
ASSERT_EQ(threadId1, threadId2);
})
.wait();
}
```
It seems intentional per the below logic, but I don't really understand as it is not inline behavior:
https://github.com/facebook/folly/blob/main/folly/futures/detail/Core.cpp#L578-L584
```
auto currentKeepAlive = std::move(currentExecutor).stealKeepAlive();
if (addCompletingKA.get() == currentKeepAlive.get()) {
keepAliveFunc(std::move(currentKeepAlive));
} else {
std::move(currentKeepAlive).add(std::move(keepAliveFunc));
}
```

I don't see many tests around `Future::thenInline` in our codebase, seems like the behavior is not well-defined yet.

Thanks.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.