Fix FunctionScheduler::resetFunctionTimer concurrency bug
authorYedidya Feldblum <yfeldblum@fb.com>
Fri, 27 Oct 2017 04:24:48 +0000 (21:24 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Fri, 27 Oct 2017 04:47:22 +0000 (21:47 -0700)
commitd89c342ab6b905d2a6bfba7317eac41d44077d65
treea402cdd02e9df9bca50fde584fd477b85b039b81
parent7ec1449d644773aacec801e473ca84438d1d0dec
Fix FunctionScheduler::resetFunctionTimer concurrency bug

Summary:
[Folly] Fix `FunctionScheduler::resetFunctionTimer` concurrency bug.

The original code from the blamed diff code has a comment with this correct assessment of the bug:
> TODO: This moves out of RepeatFunc object while folly:Function can potentially be executed. This might be unsafe.

Namely, when the method is invoked with the id of a scheduled function which is concurrently being executed, the function object (including, say, lambda captures) will be moved while possibly being accessed. If the function object is small enough to be placed in-situ within `folly::Function` (48 bytes on x64), then that access to a moved-from object can happen. It might or might not, depending on the particular instance of the race in question. Or, worse, the access might be to a half-moved-from object!

The new test case for `resetFunctionTimer` passes after the fix, but is guaranteed to fail before the fix because we manage to control the concurrency enough to force the bad version of the race to happen. In the test, we just capture a `std::shared_ptr` (we could have capatured, e.g., a `std::unique_ptr` or a long-enough `std::string`) and check that it is not empty - if it is moved from, it will be empty, and the test expectation will fail.

Reviewed By: simpkins

Differential Revision: D6158722

fbshipit-source-id: 33a7ae699bb3b22089fddbebb6d922737668309d
folly/experimental/FunctionScheduler.cpp
folly/experimental/test/FunctionSchedulerTest.cpp