1. 12 Nov, 2020 3 commits
    • Lee Howes's avatar
      Add retryingUnsafe · e5bb59b3
      Lee Howes authored
      Summary: Add retryingUnsafe as a duplicate of retrying that forces the return type to be a Future rather than a SemiFuture.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24707194
      
      fbshipit-source-id: cb2a107f51d4d4dd47a41ce6f8a009c4d82e3138
      e5bb59b3
    • Michel Salim's avatar
      add shared library support to add_fbthrift_cpp_library · cfa6e9c6
      Michel Salim authored
      Summary:
      `add_fbthrift_cpp_library` should honor `BUILD_SHARED_LIBS`, and call
      `add_library` with the right setting (`SHARED` if enabled, `STATIC` otherwise)
      
      Reviewed By: yns88
      
      Differential Revision: D24911124
      
      fbshipit-source-id: 79df7640a758a592a3df3e9e79bb129dd57f2d47
      cfa6e9c6
    • Luca Niccolini's avatar
      add zlib as an explicit dependency for getdeps build · d4340c0a
      Luca Niccolini authored
      Summary:
      needed for QUIC/H3 interop
      
      also make it possible to run getdeps build with extra arguments (--no-tests for
      example)
      
      Reviewed By: mjoras
      
      Differential Revision: D24925777
      
      fbshipit-source-id: fbdc1aa56e398d295ef8dac0ad0bab03bd7bd803
      d4340c0a
  2. 11 Nov, 2020 1 commit
  3. 10 Nov, 2020 3 commits
    • Davide Cavalca's avatar
      proxygen: fix shared libs build · bfe27697
      Davide Cavalca authored
      Summary:
      Right now proxygen hardcodes a static build when using cmake  and
      ignores BUILD_SHARED_LIBS. Fix that, and enable PIE on the shared libs so they
      can be linked properly
      
      Closes: https://github.com/facebook/proxygen/issues/335
      
      Reviewed By: mjoras, lnicco
      
      Differential Revision: D24787944
      
      fbshipit-source-id: 7a654af7cb43227ca913a1bed67f2432703a343d
      bfe27697
    • Michel Salim's avatar
      also install executor.h · 6cd0c17d
      Michel Salim authored
      Summary: This header is needed by python/futures.h
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24861734
      
      fbshipit-source-id: 913ad13e6ca155250238020538af762c2360f137
      6cd0c17d
    • Dan Melnic's avatar
      Allow recycling of std::unique_ptr<IOBuf> · d3489f9e
      Dan Melnic authored
      Summary: Allow recycling of std::unique_ptr<IOBuf>
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24650191
      
      fbshipit-source-id: 85d219052cc62c35098085abb3eed6cfe00beefc
      d3489f9e
  4. 09 Nov, 2020 4 commits
    • Michel Salim's avatar
      fix Python binding installation · 4087512f
      Michel Salim authored
      Summary:
      Honor `DESTDIR` on Unix-like platforms (important when building packages),
      and use `CMAKE_INSTALL_PREFIX` instead of the default prefix.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24821935
      
      fbshipit-source-id: 42a311f920ed74760597f8908cd339b914ecb92e
      4087512f
    • Eric Niebler's avatar
      exception_wrapper::handle should accommodate noexcept lambdas in C++17 · 477a4c23
      Eric Niebler authored
      Summary: `exception_wrapper::handle` uses the signature of `Fn::operator()` to pick apart the function type and deduce the exception type for the handler. We use specializations of an `arg_type_` class template to do this. In C++17, `noexcept` is a part of the type system, but there were no specializations of `arg_type_` to handle `noexcept`-qualified callables.
      
      Reviewed By: ispeters
      
      Differential Revision: D24733871
      
      fbshipit-source-id: 344e735e653296c9344b6c09662fccd94953e8a7
      477a4c23
    • Daan De Meyer's avatar
      Add Decider argument to retryN and retryWithExponentialBackoff · 11440055
      Daan De Meyer authored
      Summary:
      This allows customizing the exceptions for which retry
      is triggered. This is useful when we only want to retry
      in case of a few specific exceptions but have others
      cause the operation to fail immediately.
      
      Reviewed By: lewissbaker
      
      Differential Revision: D24766615
      
      fbshipit-source-id: f97451673f575bef511399cbde6c1ad110f9493a
      11440055
    • Laurent Stacul's avatar
      Fix missing #include <limits> (#1482) · 9939b376
      Laurent Stacul authored
      Summary:
      Hello,
      The compilation with the HEAD of gcc 11 fails:
      ```
      FAILED: CMakeFiles/folly_base.dir/folly/TimeoutQueue.cpp.o
      g++  ... -c ../folly/TimeoutQueue.cpp
      ../folly/TimeoutQueue.cpp: In member function 'int64_t folly::TimeoutQueue::nextExpiration() const':
      ../folly/TimeoutQueue.cpp:39:32: error: 'numeric_limits' is not a member of 'std'
         39 |       timeouts_.empty() ? std::numeric_limits<int64_t>::max()
            |                                ^~~~~~~~~~~~~~
      ../folly/TimeoutQueue.cpp:39:54: error: expected primary-expression before '>' token
         39 |       timeouts_.empty() ? std::numeric_limits<int64_t>::max()
            |                                                      ^
      ../folly/TimeoutQueue.cpp:39:57: error: '::max' has not been declared
         39 |       timeouts_.empty() ? std::numeric_limits<int64_t>::max()
            |                                                         ^~~
      ../folly/TimeoutQueue.cpp:39:57: note: suggested alternatives:
      In file included from /opt/1A/toolchain/x86_64-v21.0.8/include/c++/11.0.0/functional:65,
                       from ../folly/TimeoutQueue.h:31,
                       from ../folly/TimeoutQueue.cpp:17:
      /opt/1A/toolchain/x86_64-v21.0.8/include/c++/11.0.0/bits/stl_algo.h:3464:5: note:   'std::max'
       3464 |     max(initializer_list<_Tp> __l, _Compare __comp)
            |     ^~~
      ```
      It is nothing to fix.
      Regards,
      Laurent
      
      Pull Request resolved: https://github.com/facebook/folly/pull/1482
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24757105
      
      Pulled By: Orvid
      
      fbshipit-source-id: 8a5382edbe5b7c11cace5a1cd4c0645dfd2a9d37
      9939b376
  5. 07 Nov, 2020 2 commits
  6. 06 Nov, 2020 4 commits
    • Koray Polat's avatar
      Add an option to specify lfs path · 7ddffe80
      Koray Polat authored
      Reviewed By: rsunkad
      
      Differential Revision: D24750170
      
      fbshipit-source-id: 5c48ab812b5438a33713315faf83e7a21a3c4eae
      7ddffe80
    • TJ Yin's avatar
      fix tsan unit-test failure by reducing iteration · a6098259
      TJ Yin authored
      Reviewed By: yfeldblum
      
      Differential Revision: D24724575
      
      fbshipit-source-id: 2d7c6dbf94ee27638c92abfb94d8084beabb3d5e
      a6098259
    • Yi Zhang's avatar
      Record owner gettid() if TrackThreadId is enabled · 1540c39c
      Yi Zhang authored
      Summary:
      Adds a new TrackThreadId template argument to SharedMutexImpl that when enabled, expands SharedMutex from 4 to 8 bytes (still with
      4 byte alignment), and uses the extra space to record the thread ID
      of a distinguished owning thread (upgrade or exclusive lock holder).
      This dramatically simplifies debugging in some scenarios.  It adds enough
      information that we could enforce that unlock happens on the same thread
      as lock, but this diff doesn't actually add such a check.
      
      A new TrackedSharedMutex class is added for SharedMutex with lock owner thread id tracking enabled. fb_localtime is the only user for now to reduce the risk.
      
      Differential Revision: D24731513
      
      fbshipit-source-id: 6b41fb05498842224feb088b1332794281b501a9
      1540c39c
    • Lee Howes's avatar
      Move retrying parameter to match old behaviour · dd4c59a9
      Lee Howes authored
      Summary: The old behaviour passed an r-value after incrementing. Some use cases take an r-value and hence depend on that behaviour. This change makes that consistent.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24634859
      
      fbshipit-source-id: 872197a11197a5e60f32492f1670dd94cf78c8b8
      dd4c59a9
  7. 05 Nov, 2020 6 commits
    • Lukas Piatkowski's avatar
      rust-shed/futures_01_ext: rename futures_ext to futures_01_ext · b519e71b
      Lukas Piatkowski authored
      Summary: As part of the effort to deprecate futures 0.1 in favor of 0.3 I want to create a new futures_ext crate that will contain some of the extensions that are applicable from the futures_01_ext. But first I need to reclame this crate name by renaming the old futures_ext crate. This will also make it easier to track which parts of codebase still use the old futures.
      
      Reviewed By: farnz
      
      Differential Revision: D24725776
      
      fbshipit-source-id: 3574d2a0790f8212f6fad4106655cd41836ff74d
      b519e71b
    • Wez Furlong's avatar
      getdeps: don't depend on git fetch depth any longer · 079cd3cc
      Wez Furlong authored
      Summary:
      This commit takes advantage of git 2.5.0 being able to fetch a
      requested revision rather than relying on the desired revision being within the
      depth limited fetch.
      
      This relies on having git 2.5.0 on the server which is true for all
      of the projects we have manifests for; this shows zero matches:
      
      ```
      $ rg repo_url opensource/fbcode_builder/manifests | grep -v github
      ```
      
      We've had a couple of situations recently where folks have run into issues with
      the commit rate in folly being higher than then fetch depth, so this should
      address that.
      
      Refs: https://github.com/facebook/watchman/issues/866
      
      Reviewed By: fanzeyi
      
      Differential Revision: D24747992
      
      fbshipit-source-id: e9b67c61dddc9f55e05d8984e8d210e7d2faabcb
      079cd3cc
    • Andrii Grynenko's avatar
      Make coro::sleep throw when cancelled · 8dfce3ec
      Andrii Grynenko authored
      Summary: We shouldn't be sleeping less that requested and return silently.
      
      Reviewed By: lewissbaker
      
      Differential Revision: D24633977
      
      fbshipit-source-id: 71ef422f0f72747bc19ac8491fdb8b148598d499
      8dfce3ec
    • Junqi Wang's avatar
      Fix tsan warning in AsyncUDPSocket test · 244b6f44
      Junqi Wang authored
      Summary:
      tsan reports warning in the test because the server socket writes in
      a thread but closes in another thread without synchronization. This was by
      design and is not a bug since implicit synchronization is done by joining client
      thread, but this isn't visible to tsan. The fix is to use a port reused new
      server socket to write response to client.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24675113
      
      fbshipit-source-id: bdab8a5d5b6f85aa36ba84a6299e4457710625de
      244b6f44
    • Alexey Spiridonov's avatar
      Fix OSS build · aaf2076d
      Alexey Spiridonov authored
      Summary:
      There are two separate changes here.
      
      ### Use `find_package`
      
      The old setup of "let's manually enumerate and order the libraries that Bistro depends on" worked fine, except:
       - it was a bit brittle (requiring occasional patches as deps changed), and
       - it garnered a lot of feedback to the effect of "your build is weird, so it's probably broken because of that."
      
      Now I expect to have fewer breaks and more plausible deniability :)
      
       More importantly, this should make it much easier to migrate to `getdeps.py`.
      
      ## Statically link `fmt`
      
      After `fmt` was added as a `folly` dependency, and linked into Folly code used by Bistro, its tests would fail to run with this error: `test_sqlite_task_store: error while loading shared libraries: libfmt.so.6: cannot open shared object file: No such file or directory`.
      
      Something was getting messed up in the dynamic linking, and it wasn't clear to me what -- the way that Bistro is linking its dependencies certainly seems sensible. Most likely one of the dependencies is incompatible with dynamic linking in a subtle way. I suspect Proxygen.
      
      The `fmt.py` change in this diff addresses this problem by forcing static linking on the offending library.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24604309
      
      fbshipit-source-id: 35ecbbb277b25907ecaee493e8b0081d9f20b865
      aaf2076d
    • Alexey Spiridonov's avatar
      Fix discovery of `libsodium` · 74ed883e
      Alexey Spiridonov authored
      Summary:
      By putting this in `fizz-config.cmake`, we can use depend on the `sodium` target without compromising our dependents ability to find the library.
      
      Put the search module in the common location under `fbcode_builder/CMake` to let dependents use it.
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24686041
      
      fbshipit-source-id: 942d1ab34feef6cadac2b584eb8cb2d999bae0ca
      74ed883e
  8. 04 Nov, 2020 3 commits
    • Yedidya Feldblum's avatar
      Use ::new and std::addressof in Try · 6bb79f51
      Yedidya Feldblum authored
      Summary: [Folly] Use `::new` and `std::addressof` in `Try` to evade custom overloads of `operator new` and of `operator&` in certain types.
      
      Reviewed By: lewissbaker
      
      Differential Revision: D24698131
      
      fbshipit-source-id: 9b4a3a5e9962018450104ee0d6df2cf58a2881b7
      6bb79f51
    • Francis Ricci's avatar
      Add mmap safety check to the elf file parser · 120fb0fd
      Francis Ricci authored
      Summary: mmap() calls are a bit dangerous, in that the backing files can be corrupted or unreadable. Ideally, we would just migrate this whole symbolizer to use ::pread(), but in the meantime, just making sure that the last byte of the mmap'd file is readable seems to get rid of the crashes we've been seeing.
      
      Reviewed By: ccdavid
      
      Differential Revision: D24505608
      
      fbshipit-source-id: e7bb3ea4d34f7b79f5a1cbb593a195f836d85ec7
      120fb0fd
    • TJ Yin's avatar
      fix gnu_debuglink_test during parallel test execution · cda620cb
      TJ Yin authored
      Summary: This diff appended uuid to copied files so that we won't run into race condition when 2 tests are executed at the same time.
      
      Reviewed By: meyering
      
      Differential Revision: D24589043
      
      fbshipit-source-id: edb6b6eb6c8749694bdad15eb8d00d9f8d931cfc
      cda620cb
  9. 02 Nov, 2020 4 commits
    • John Kearney's avatar
      Add virtual destructor to SequencedExecutor · 7feb2d37
      John Kearney authored
      Summary: - Fix linker error when building with clang on windows
      
      Reviewed By: yfeldblum, jdonald
      
      Differential Revision: D24683230
      
      fbshipit-source-id: fe9e5daebb058e007d8f5725ef31e62967a94f15
      7feb2d37
    • Nicholas Ormrod's avatar
      Rm std::unique_ptr null-deleter test case · 2834b6e4
      Nicholas Ormrod authored
      Summary:
      The null-deleter test case showcases not_null's handling of null deleters. It also tries to demonstrate that std::unique_ptr does not handle null deleters properly (e.g. by segfaulting). The latter is really "check what kind of undefined behavior we encounter", and this doesn't always work. Surprise! Specifically, the undefined behavior encountered when supplying a null deleter into std::unique_ptr when using tsan is an infinite hang, which breaks the test suite.
      
      Fortunately, testing std::unique_ptr's handling of null deleters is irrelevant to the correctness of not_null_unique_ptr, so we can remove this code.
      
      After this diff, the following test does not hang:
        buck test mode/dev-tsan //folly/memory/test:not_null_test -- 'nn\.null_deleter' --run-disabled
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24662902
      
      fbshipit-source-id: 230187890b29e1d1a5c8284958c9b798c891c7f7
      2834b6e4
    • Dan Melnic's avatar
      Fix heap use after free test issue · 98625fc8
      Dan Melnic authored
      Summary: Fix heap use after free test issue
      
      Reviewed By: danobi
      
      Differential Revision: D24656212
      
      fbshipit-source-id: 03ab1ee563f56e55a7b793155355c43bdfa11fdf
      98625fc8
    • Kyle Nekritz's avatar
      Add mocks for bytes buffered getters. · abeb13b7
      Kyle Nekritz authored
      Reviewed By: yfeldblum
      
      Differential Revision: D24660041
      
      fbshipit-source-id: fe31b99206d5bf5b643d8e8111b25c570fe7c0d8
      abeb13b7
  10. 31 Oct, 2020 1 commit
    • Aaryaman Sagar's avatar
      Disable an exception-throw stress test for DistributedMutex under TSAN · c7245ac0
      Aaryaman Sagar authored
      Summary:
      This test inexplicably aborts when ran under TSAN.  TSAN reports a read-write
      race where the exception is freed by the lock-holder / combiner thread and read
      concurrently by the thread that requested the combine operation.
      
      TSAN reports a similar read-write race for this code as well
      https://gist.github.com/aary/a14ae8d008e1d48a0f2d61582209f217 (copied below).
      Which is easier to verify as being correct.  Though this does not conclusively
      prove that this test and implementation are race-free, the above repro combined
      with error-free stress runs of this test under other modes (optimized builds
      with and without both ASAN and UBSAN) give us a high signal at TSAN's error
      being a false-negative.
      
      So disabling it for now until some point in the future where TSAN stops
      reporting this as a false-negative
      
      ```
      namespace {
      struct Storage {
        std::atomic<std::uint64_t> futex_{0};
        std::atomic<std::uint64_t> next_;
        std::aligned_storage_t<48, 8> storage_;
      };
      
      template <typename Waiter>
      void transferCurrentException(Waiter* waiter) {
        assert(std::current_exception());
        new (&waiter->storage_) std::exception_ptr{std::current_exception()};
        waiter->futex_.store(1, std::memory_order_release);
      }
      
      template <template <typename> class Atom = std::atomic>
      void concurrentExceptionPropagationStress(
          int,
          std::chrono::milliseconds) {
        auto exceptions = std::vector<std::unique_ptr<Storage>>{};
        exceptions.resize(1000000);
        for (auto i = std::size_t{0}; i < exceptions.size(); ++i) {
          exceptions[i] = std::make_unique<Storage>();
        }
        auto throwing = std::function<void(std::uint64_t)>{[&](auto counter) {
          if (counter % 2) {
            throw std::runtime_error{folly::to<std::string>(counter)};
          }
        }};
      
        std::cout << "Started test" << std::endl;
        auto writer = std::thread{[&]() {
          for (auto i = std::size_t{0}; i < exceptions.size(); ++i) {
            try {
              std::this_thread::yield();
              throwing(i);
            } catch (...) {
              transferCurrentException(exceptions.at(i).get());
              continue;
            }
      
            exceptions.at(i)->futex_.store(2, std::memory_order_release);
          }
        }};
      
        auto reader = std::thread{[&]() {
          for (auto i = std::size_t{0}; i < exceptions.size(); ++i) {
            auto value = std::uint64_t{};
            while (!(value = exceptions.at(i)->futex_.load(std::memory_order_acquire))) {}
      
            if (value == 1) {
              try {
                auto buf = reinterpret_cast<std::exception_ptr*>(&exceptions.at(i)->storage_);
                auto ex = folly::launder(buf);
                auto copy = std::move(*ex);
                ex->exception_ptr::~exception_ptr();
                std::rethrow_exception(std::move(copy));
              } catch (std::exception& exc) {
                assert(folly::to<std::uint64_t>(exc.what()) == i);
              }
            }
          }
        }};
      
        reader.join();
        writer.join();
      }
      } // namespace
      ```
      
      Reviewed By: yfeldblum
      
      Differential Revision: D24653383
      
      fbshipit-source-id: 3ce166766eb42b22ec7b8cfaf8d31ca53580ff9e
      c7245ac0
  11. 30 Oct, 2020 6 commits
    • Lewis Baker's avatar
      Fix co_awaitTry() with Task::scheduleOn() · e5676873
      Lewis Baker authored
      Summary:
      Fixes compilation of `co_await co_awaitTry(someTask.scheduleOn(ex))` when
      awaited within other `Task` coroutines by adding the `await_resume_try()`
      method to ViaIfAsyncAwaiter.
      
      Also added some unit-tests for this case.
      
      Reviewed By: andriigrynenko
      
      Differential Revision: D24650827
      
      fbshipit-source-id: 687717d9f73f1fdcfb1f2ff17e4ddca46c16b189
      e5676873
    • Dan Melnic's avatar
      Fix async socket test (stack-use-after-scope) · 31bd58d8
      Dan Melnic authored
      Summary: Fix async socket test (stack-use-after-scope)
      
      Reviewed By: andriigrynenko
      
      Differential Revision: D24650836
      
      fbshipit-source-id: 795fb71770612a409195d93216c7c5e129bea605
      31bd58d8
    • Dan Melnic's avatar
      Avoid dereferencing a nullptr socket_ · c3fe90d8
      Dan Melnic authored
      Summary: Avoid dereferencing a nullptr socket_
      
      Reviewed By: danobi
      
      Differential Revision: D24652875
      
      fbshipit-source-id: 63a9535226c8ff58fd5348f3c38f5c6c498d9527
      c3fe90d8
    • Matt Joras's avatar
      Allow disabling of IPV6_ONLY for AsyncUDPSocket · bd529c4c
      Matt Joras authored
      Summary: It's useful to be able to use the v4-mapped addresses sometimes.
      
      Reviewed By: avasylev
      
      Differential Revision: D24371807
      
      fbshipit-source-id: 74fe13fa4bef9c77cc51a18137559facda9bdbeb
      bd529c4c
    • Lewis Baker's avatar
      Add support for co_awaitTry(task.scheduleOn()) · 505affdc
      Lewis Baker authored
      Summary:
      This adds support for applying the co_awaitTry() algorithm to a
      TaskWithExecutor type.
      
      This required a bit of a refactor of the `co_awaitTry()` implementtion
      to not require that it's passed a SemiAwaitable but and to allow
      Awaitable objects to also be passed to `co_awaitTry()`.
      
      Renamed TaskWithExecutor::InlineAwaiter to InlineTryAwaitable and make
      it private to better reflect its purpose.
      
      No longer implement `Task::Awaiter::await_resume()` in terms of the
      `await_resume_try()` method. This should avoid a call to the Try<T>
      move-constructor by extracting the T result directly from the Try<T>
      stored in the promise.
      
      Reviewed By: andriigrynenko
      
      Differential Revision: D24602729
      
      fbshipit-source-id: d352e717877769502cc55a01731eb165ddad9ad8
      505affdc
    • Andrii Grynenko's avatar
      Template AtomicNotificationQueue by Task and Consumer · 018a9487
      Andrii Grynenko authored
      Summary: This allows migrating projects that were creating custom NotificationQueues.
      
      Differential Revision: D24580356
      
      fbshipit-source-id: ce9a15c408224b8a471246fde2bd121eab4ae329
      018a9487
  12. 29 Oct, 2020 3 commits
    • Dan Melnic's avatar
      AsyncUDPSocket zerocopy support · 2109302c
      Dan Melnic authored
      Summary:
      AsyncUDPSocket zerocopy support
      
      (Note: this ignores all push blocking failures!)
      
      Reviewed By: mjoras
      
      Differential Revision: D24553259
      
      fbshipit-source-id: 8c2385a2a336f3c84c76e4e29f42596a5af657e7
      2109302c
    • Shawn Wu's avatar
      EliasFano: fix an edge case in the conditional check during encoding. · 93ee3410
      Shawn Wu authored
      Summary:
      Context: we don't yet support uint8_t/uint16_t as SkipValueType due to integral promotion errors. Once we do (something like D22249713, D24571536), consider the below test case
      
      ```
      using ValueType = uint64_t;
      using SkipValueType = uint8_t;
      Encode<ValueType, SkipValueType, ...>({0, std::numeric_limits<ValueType>::max() -1});
      ```
      
      This would check fail at
      ```
      CHECK_LT(upperBound >> numLowerBits, std::numeric_limits<SkipValueType>::max());
      ```
      
      (255 vs 255)
      
      In fact, I think CHECK_LE is a more correct check.
      
      Also made another check fail to print nicer results for uint8 values.
      
      Reviewed By: ot, philippv
      
      Differential Revision: D24605502
      
      fbshipit-source-id: e422ed1f959ff8ef63dfe99149340637a3741b2a
      93ee3410
    • Adam Simpkins's avatar
      update the README build status badges to point to Github Actions · ae7f09e3
      Adam Simpkins authored
      Summary:
      Replace the Travis build status badge in README.md with 3 separate badges
      for the newer Linux, Mac, and Windows builds performed via Github Actions.
      
      Reviewed By: yfeldblum, vitaut
      
      Differential Revision: D24610866
      
      fbshipit-source-id: 6581387989fb15213519db7810a4430cfb04e2ea
      ae7f09e3