Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update #1

Merged
merged 1,043 commits into from
Mar 4, 2019
Merged

update #1

merged 1,043 commits into from
Mar 4, 2019

Conversation

sandraiser
Copy link
Owner

No description provided.

Nathan Bronson and others added 30 commits December 28, 2018 07:26
Summary:
Previously the debug-build randomization of F14 iteration order
was applied only to F14ValueMap/Set, F14NodeMap/Set, and F14FastMap/Set
that uses the value storage strategy.  This extends the behavior to
F14FastMap/Set that use the vector storage strategy, which are those
instances where sizeof(value_type) >= 24.

F14FastMap/Set using the vector storage strategy must move items
to randomize, so this reordering will also expose cases that assume
reference or iterator stability across multiple inserts without a call
to .reserve().

Reviewed By: yfeldblum

Differential Revision: D13305818

fbshipit-source-id: 178a1f7b707998728a0451af34269e735bf063f3
Summary:
[Folly] `test_once`, a way to check whether any call to `call_once` with a given `once_flag` has succeeded.

One example of use might be for exception-safety guarding object destruction when object construction is guarded by the `once_flag`, and when the user is interested in conserving per-object memory and wishes to avoid the extra 8-byte overhead of `std::optional`.

```lang=c++
template <typename T>
struct Lazy {
  folly::aligned_storage_for_t<T> storage;
  folly::once_flag once;

  ~Lazy() {
    if (folly::test_once(once)) {
      reinterpret_cast<T&>(storage).~T();
    }
  }

  template <typename... A>
  T& construct_or_fetch(A&&... a) {
    folly::call_once(once, [&] { new (&storage) T(std::forward<A>(a)...); });
    return reinterpret_cast<T&>(storage);
  }
};
```

Reviewed By: ovoietsa

Differential Revision: D13561365

fbshipit-source-id: 8376c154002f1546f099903c4dc6be94dd2def8e
Summary:
Prevent deadlock on tagged retired lists within calls to `do_reclamation()` that call `cleanup_batch_tag()`.

Changes:
- Make locking the tagged list reentrant.
- Eliminate sharding of tagged lists to prevent deadlock between concurrent calls to `do_reclamation()` on different shards that call `cleanup_batch_tag` on the other shard.
- Expose the list of unprotected objects being reclaimed in `do_reclamation()` of the tagged list so that calls to `cleanup_batch_tag()` don't miss objects with a matching tag.
- Refactor of commonalities between calls to `retire()` in `hazptr_obj_base` and `hazptr_obj_base_linked` into `hazptr_obj::push_obj()`.
- Fixed release of tagged list lock to use CAS in a loop instead of store, since concurrent lock-free pushes are possible.

Reviewed By: davidtgoldblatt

Differential Revision: D13439983

fbshipit-source-id: 5cea585c577a64ea8b43a1827522335a18b9a933
Summary: Enable destruction order guarantee, i.e., destructors for all key and value instances will complete before the completion of the destructor of the associated ConcurrentHashMap instance.

Reviewed By: davidtgoldblatt

Differential Revision: D13440153

fbshipit-source-id: 21dce09fa5ece00eaa9caf7a37b5a64be3319d5e
Summary: Add test for recursive destruction.

Reviewed By: djwatson

Differential Revision: D13476864

fbshipit-source-id: 513f39f44ad2f0d338d10066b2e337902db32e00
Summary:
In the case when an explicit capacity is specified (via reserve()
or an initial capacity) we can save memory by using a bucket_count()
off of the normal geometric sequence.  This is beneficial for sizes
<= Chunk::kCapacity for all policies, and for F14Vector tables with
any size.  In the multi-chunk F14Vector case this will save about
40%*size()*sizeof(value_type) when reserve() is used (such as during
Thrift deserialization).  The single-chunk savings are potentially larger.
The changes do not affect the lookup path and should be a tiny perf win in
the non-growing insert case.  Exact sizing is only attempted on reserve()
or rehash() when the requested capacity is >= 9/8 or <= 7/8 of the current
bucket_count(), so it won't trigger O(n^2) behavior even if misused.

Reviewed By: yfeldblum

Differential Revision: D12848355

fbshipit-source-id: 4f70b4dabf626142cfe370e5b1db581af1a1103f
Summary: Everything has been migrated over to the NetworkSocket overload.

Reviewed By: yfeldblum

Differential Revision: D13566609

fbshipit-source-id: 920505a9e91f1acc5810949049880ed07294621b
Summary:
[Folly] Make `co_current_executor` look like `nullptr`, `std::nullopt`, `std::in_place`.

* Use a `co_` prefix to indicate that it is offers a useful result when awaited.
* Offer a well-named value with a well-named type or type alias.
  * There is the `nullptr` value and `std::nullptr_t` type or type alias.
  * There is the `std::nullopt` value and the `std::nullopt_t` type or type alias.
  * There is the `std::in_place` value and the `std::in_place_t` type or type alias.

Reviewed By: andriigrynenko, lewissbaker

Differential Revision: D13561713

fbshipit-source-id: 835da086e7165d37a952a1f169318cb566401d12
Summary:
[Folly] Generic detection of empty-callable in `Function` ctor.

Constructing a `folly::Function` from an empty `std::function` should result in an empty object. However, it results in a full object which, when invoked, throws `std::bad_function_call`. This may be a problem in some cases which need to use the emptiness/fullness property to tell whether `std::bad_function_call` would be thrown if the object were to be invoked.

This solution proposes a new protocol: to check arguments of all types, not just pointers, for constructibility-from and equality-comparability-with `nullptr`, and then if those two checks pass, to check equality-comparison-with `nullptr`. If the argument type is is constructible from `nullptr` and is equality-comparable with `nullptr` and compares equal to `nullptr, then treat the argument as empty, i.e., as if it were `nullptr`. This way, an empty `std::function` gets treated as if it were `nullptr` - as well as any other custom function object type out there - without having to enumerate every one of them.

The new protocol is somewhat strict. An alternative to the new protocol is to check if the object is castable to `bool` and, if it is, cast to `bool`, but such a protocol is broader than the one proposed in this diff.

Fixes #886.

Reviewed By: nbronson

Differential Revision: D9287898

fbshipit-source-id: bcb574387122aac92d154e81732e82ddbcdd4915
Summary:
[Folly] Fix misfiring `unused-local-typedef` under clang.

The problem is fixed in most recent versions of clang, but appears with some previous versions.

```
folly/experimental/coro/Task.h:318:11: error: unused type alias 'handle_t' [-Werror,-Wunused-local-typedef]
    using handle_t =
          ^
```

Reviewed By: andriigrynenko

Differential Revision: D13569416

fbshipit-source-id: 94ae07455c2d08a1516c10baf1e3a16f2a29225f
Summary: [Folly] Support `-fno-exceptions` in `folly/small_vector.h`.

Reviewed By: ot

Differential Revision: D13499417

fbshipit-source-id: d1b50ff7f028203849888f42a44c9370986a7ac1
Summary:
Currently if `cancelAll` is called from inside the `timeoutExpired` of one of the callbacks, it will not cancel timeouts that we're about to run (they were extracted from the buckets already).

This diff fixes that behavior by also canceling timeouts in `timeoutsToRunNow_` list (note, we were already doing that in the destructor).

Reviewed By: yfeldblum

Differential Revision: D13531908

fbshipit-source-id: f05ba31f2ac845851c1560d2ebdf41aa995b2deb
Summary:
Currently, every timeout scheduling will compute the next tick twice:
* when deciding which bucket to use
* when actually scheduling next timeout in `scheduleNextTimeout`

This means that we may end up using a bigger nextTick in `scheduleNextTimeout` (if we were at a boundary of a tick and/or if there was a context switch). This means that if the timeout value is low and we will put it for `nextTick`, then it will end up waiting 2.5s (while we make one round over all slots).

With this change we make sure that the same tick is used.

I could consistently reproduce an issue by running 256 threads that just schedule immediate timeouts all the time.

Reviewed By: yfeldblum

Differential Revision: D13531907

fbshipit-source-id: a152cbed7b89e8426b2c52281b5b6e171e4520ea
Summary:
The current implementation may fire timeouts prematurely due to them being put in a bucket that we're about to run. In particular, timeouts that span `WHEEL_SIZE-1` (2.55-2.56s with default interval of 10ms) have very high likelihood of being fired immediately if we have another callback already scheduled.

The issue is that we may use a bucket for the next epoch of wheel before the previous epoch drained it. This diff fixes it by using next unprocessed bucket as a base.

Reviewed By: yfeldblum, djwatson

Differential Revision: D13541502

fbshipit-source-id: 963139e77615750820a63274a1e21929e11184f1
Summary:
Currently, when cascading, we are unable to use bucket 0. This means that timeouts that are already expired would need to wait another tick before being fired.

A simple example is when we're at tick 0 and scheduling for tick 256, such timeout would go into cascading logic and should fall into bucket 0 of next wheel epoch. However, the existing logic would cascade it to bucket 1.

This diff fixes that, by reordering draining of current bucket and cascading timeouts.

Reviewed By: yfeldblum

Differential Revision: D13541506

fbshipit-source-id: 1284fca18612ae91f96538192bfad75e27cd816c
Summary:
Current logic will use last processed tick as next tick if we are inside of timeout handling.

However, this is wrong in two ways:
* cascading logic would compute number of ticks until expiration from real current time, however it will use `lastTick_` as base and thus may lead to premature timeout.
* if we spent non-trivial amount of time invoking callbacks, we may use stale tick number as base of some timeout.

Reviewed By: djwatson

Differential Revision: D13541504

fbshipit-source-id: b7e675bb5a707161f5c7f636d4c2a374a118da83
Summary: There's no need to distinguish between them (it only adds complexity by having two modes: schedules vs running).

Reviewed By: djwatson

Differential Revision: D13541505

fbshipit-source-id: 7cb2c3fb9ba4e3b191adee37ad1d28f471378f85
…ogic

Summary: There's no need to extensively call into steady_clock, because the time is unlikely to change. And if it does change, then it means that we'd do incorrect math (like firing timeout 10ms earlier).

Reviewed By: djwatson, stevegury

Differential Revision: D13541503

fbshipit-source-id: cc46c8a6bd6a72544803a22d16235be54fa94cc4
Summary: [Folly] Add a missing blank line (style nit).

Reviewed By: lewissbaker

Differential Revision: D13571222

fbshipit-source-id: 1dd23f4fc895e5698f94be6b2cbf90a9f30aae41
Summary: Add AsyncUDPSocket support for sendmmsg

Reviewed By: djwatson

Differential Revision: D13521601

fbshipit-source-id: 89382e18943e01012ff1e56a40f655d634a6e146
Summary:
Because it wasn't clear for non-python developers. And this document is referenced in Spark AR documentation.
Pull Request resolved: #988

Reviewed By: yfeldblum

Differential Revision: D13570180

Pulled By: Orvid

fbshipit-source-id: 2e1f787c5e1bb50a90c85a12e6801a79a3b46999
Summary:
Adds support for zstd-1.3.8 so OSS builds work.
We can support zstd < 1.3.8 for some time with this small compatibility layer.
I plan on always supporting at minimum the latest 2 zstd versions.

Reviewed By: yfeldblum

Differential Revision: D13569550

fbshipit-source-id: 67d53c9ad0051a889b810c9ad46a2f349122cf7e
Summary:
This diff imports farmhash.h and farmhash.cc from
https://github.com/google/farmhash and updates the public_tld license
file accordingly. Build integration and namespace changes will occur in
later diffs.

Reviewed By: yfeldblum

Differential Revision: D13436553

fbshipit-source-id: 7a081032cb35a1a3e1cd14e2edf2685906956396
Summary:
Skip F14Map.continuousCapacity* tests if intrinsics are not
available, and don't use c++17 API in the test.

Reviewed By: yfeldblum

Differential Revision: D13575479

fbshipit-source-id: 1cbbd10990ba5f0cc64ad1b29d4701b700dd16be
Summary: As interim steps to codemod to.

Reviewed By: yfeldblum

Differential Revision: D13568657

fbshipit-source-id: b143b5bab0a64c196892358a30fce17037b19b21
Summary:
Adds a simple SharedMutex type to the folly::coro namespace.

The `co_[scoped_]lock[_shared]()` methods return semi-awaitables that require the caller to provide an executor to resume on in the case that the lock could not be acquired synchronously. This avoids some potential issues that could occur if the `.unlock()` operation were to resume awaiting coroutines inline.

If you are awaiting within a `folly::coro::Task` then the current executor is implicitly provided. Otherwise, the caller can explicitly provide an executor by calling `.viaIfAsync()`.

The implementation has not been optimised and currently just relies on a `SpinLock` to synchronise access to internal state.

The main aim for this change is to make available a SharedMutex abstraction with the desired API that applications can start writing against which we can later optimise as required.

Reviewed By: andriigrynenko

Differential Revision: D9995286

fbshipit-source-id: aa141ad241d29daff2df5f7296161517c99ab8ef
Summary:
Before this change one could write:
  SemiFuture<void> f() {
    co_await f1();
    co_await f2();
  }
where SemiFuture coroutine would have semantics of an InlineTask (f1 called inline, f2 would be called on the executor which completes f1). This doesn't match the semantics of SemiFuture with deferred work (both f1() and f2() called on the executor that was passed to SemiFuture's via).

Drop support for SemiFuture coroutines, because that isn't used anywhere except for toSemiFuture function.

Reviewed By: lewissbaker

Differential Revision: D13501140

fbshipit-source-id: d77f491821e6a77cef0c92d83839bff538552b32
Reviewed By: mengz0

Differential Revision: D13580163

fbshipit-source-id: 195e3007c6cbf4bf7281435c48a9b6f6c6eada5b
…points

Summary:
The folly::coro::Task coroutine type now captures the current RequestContext when the coroutine suspends and restores it when it later resumes.

This means that folly::coro::Task can now be used safely with RequestContext and RequestContextScopeGuard.

Reviewed By: andriigrynenko

Differential Revision: D9973428

fbshipit-source-id: 41ea54baf334f0af3dd46ceb32465580f06fb37e
Summary:
Expose the IOBuf SharedInfo::userData

(Note: this ignores all push blocking failures!)

Reviewed By: yfeldblum

Differential Revision: D13577451

fbshipit-source-id: b52ebbf77d00594a04c26e629d5c208e92801d93
yfeldblum and others added 27 commits February 26, 2019 22:52
Summary: [Folly] `DefaultKeepAliveExecutor::reset` in case of restartable executors.

Reviewed By: andriigrynenko

Differential Revision: D14236173

fbshipit-source-id: 14f71af6bd54777770fdfe6036a4137af4e884bc
)

Summary:
- `folly/experimental/Bits.h` has a section where we disable two
  uninitialized-flavors of warnings that were needed for GCC 4.8
- Since we do not support GCC 4.8 anymore and GCC 5.1 does not have
  issues with this, do not disable the warnings anymore.
Pull Request resolved: #1033

Reviewed By: Orvid

Differential Revision: D14223285

Pulled By: yfeldblum

fbshipit-source-id: 0088341b13e26dff2dc90a768319cccc45e942da
…/detail/Traits.h

Summary:
Make this metafunction available for use outside of the
folly::coro::blockingWait() implementation.

I plan to reuse this metafunction for other operators such as `when_all()`.

Reviewed By: yfeldblum

Differential Revision: D13778687

fbshipit-source-id: f766f93da21bf19e89262cd60048b0b7b1b76940
Summary: I missed one of the functions that needs a NetworkSocket overload in past iterations.

Reviewed By: yfeldblum

Differential Revision: D14237211

fbshipit-source-id: 6d0bd151ff636206a44940c88a2d48c339aa67de
Summary: Merge two mutexes together and get rid of atomics.

Reviewed By: yfeldblum

Differential Revision: D14236244

fbshipit-source-id: 0b97d519e72322377e225a0cc8975e3a2a86ec31
Summary: It is dead

Reviewed By: yfeldblum

Differential Revision: D14235873

fbshipit-source-id: bafc323898f2a17aedf6644f9c2dd7319b8e6420
Summary:
- If a type is not marked explicitly with `IsRelocatable`
  as `std::true_type`, we infer its relocability only based on the
  property of whether the type is trivially copyable.
- Extend the default relocability to also be true for trivially move
  constructible types.
Pull Request resolved: #1035

Reviewed By: Orvid

Differential Revision: D14240127

Pulled By: yfeldblum

fbshipit-source-id: 1e15d312d1a8340417bba2beb1db30ce4c543b26
Summary:
Don't assume a jemalloc target exists (even if it would be a no-op, as
in on macOS). Instead compute the correct set of external_deps in
jemalloc.bzl.

This fixes the folly build on mode/mac after D14179337.

Reviewed By: andrewjcg

Differential Revision: D14235815

fbshipit-source-id: d1bfbb91897de9557524bbb4b5736ca7b03cbf9b
Summary:
InlineFunctionRef is a semantically the same as folly::FunctionRef but has the
additional benefit of being able to store the function it was instantiated
with inline in a buffer of the given capacity.  If there is not enough in-situ
capacity for the callable, this has the same semantics as FunctionRef.

This helps give a perf boost in the case where the data gets separated from
the point of invocation.  If, for example, at the point of invocation, the
InlineFunctionRef object is not cached, a remote memory/cache read might be
required to invoke the original callable.  Customizable inline storage helps
tune storage so we can store a type-erased callable with better performance
and locality.  A real-life example of this might be a folly::FunctionRef with
a function pointer.  The folly::FunctionRef would point to the function
pointer object in a remote location.  This causes a double-indirection at the
point of invocation, and if that memory is dirty, or not cached, it would
cause additional cache misses.  On the other hand with InlineFunctionRef,
inline storage would store the value of the function pointer, avoiding the
need to do a remote lookup to fetch the value of the function pointer.

To prevent misuse, InlineFunctionRef disallows construction from an lvalue
callable.  This is to prevent usage where a user relies on the callable's
state after invocation through InlineFunctionRef.  This has the potential to
copy the callable into inline storage when the callable is small, so we might
not use the same function when invoking, but rather a copy of it.

Also note that InlineFunctionRef will always invoke the const qualified
version of the call operator for any callable that is passed.  Regardless of
whether it has a non-const version.  This is done to enforce the logical
constraint of function state being immutable.

This class is always trivially-copyable (and therefore
trivially-destructible), making it suitable for use in a union without
requiring manual destruction.

Reviewed By: yfeldblum, ot

Differential Revision: D14029799

fbshipit-source-id: 2cff3ce27d564f3d524095189f847c14911f9402
Summary: Deprecate sleepUnsafe to avoid use growing and to clearly communicate the change.

Reviewed By: yfeldblum

Differential Revision: D14255404

fbshipit-source-id: cb2116910a15b9835fef9bf4273242457a5e96bd
…et() to the NetworkSocket overload

Summary: The file descriptor overload will be going away.

Reviewed By: yfeldblum

Differential Revision: D13998628

fbshipit-source-id: 7a1d3bae03d4742d874e1e7d733f7733b5a1c110
Summary: It's dead.

Reviewed By: yfeldblum

Differential Revision: D14192000

fbshipit-source-id: 3cecbc8e9f656080a023d676883de9943e7d0004
…relocatable"

Summary: Original commit changeset: 1e15d312d1a8

Reviewed By: Orvid

Differential Revision: D14265880

fbshipit-source-id: 8c2ad3e15334015dadc19bf77b061f66086adcd6
Summary:
This was treating a socket as a file descriptor, so make it explicitly be a file descriptor instead.
The full fix for this requires a *lot* of refactoring and splitting of a lot of concepts within Folly, so this short-term fix should be good enough for now.

Reviewed By: boguscoder

Differential Revision: D14266198

fbshipit-source-id: bafb3a403ab8555f865f4541323c5d0ad81f5052
Summary: Implemented the Blake2xb XOF. See the specification at https://blake2.net/blake2x.pdf.

Reviewed By: djwatson

Differential Revision: D13403920

fbshipit-source-id: 38afd6d3b782c2f6649365d01ca3d83b55c90c4b
Summary: struct crypto_generichash_blake2b_state was made opaque in libsodium 1.0.17. This change is needed to make blake2xb code compile.

Reviewed By: djwatson

Differential Revision: D14141454

fbshipit-source-id: eee887e8300229ff568d12ef19b46472edfc56d2
Summary:
Added LtHash, a cryptographic homomorphic hash, to folly/experimental/crypto.
This has a soft dependency on libsodium and the code will not be compiled if libsodium is not detected by cmake.

Reviewed By: djwatson

Differential Revision: D13390825

fbshipit-source-id: f7597ced7bcc7b403e8bbaa733837b795128f1b3
Summary: [Folly] Avoid explicit `memset` in `IPAddress` storage union by having an explicit storage field and value-initializing it in the default constructor.

Reviewed By: stevegury

Differential Revision: D14270246

fbshipit-source-id: f1f135f20709d8225e984ee3d6b397d4b04e0d76
Summary:
The `TypeError` special member functions - copy and move constructors and assignment operators - were outlined to reduce inline code size of `throw TypeError(...);` statements. This is no longer necessary, since these sites have been replaced by `folly::throw_exception<TypeError>(...);` statements, which have minimal inline code size. As a benefit, the special member functions may all now be declared as defaulted within the class body, and the conditional `noexcept` calculations are no longer required to be written - the compiler will do them. (Note: as of the libstdc++ which ships with GCC 5, library exception special member functions are `noexcept`; previously, they are not.)

Pull Request resolved: #1034

Reviewed By: nbronson

Differential Revision: D14255166

Pulled By: yfeldblum

fbshipit-source-id: 2f795a2b937fee58f243a9d374fc01829c674fe6
Summary:
[Folly] Make `EventBase` enqueue `noexcept`.

It cannot really fail anyway in correct usage besides allocation failure, unless in the `EventBase` destructor and while draining and the `AlwaysEnqueue` variant is called.

Theoretically if a caller attempts to enqueue concurrently with `EventBase` dtor while in `consumeUntilDrained`, but either *not* in the `EventBase` thread or in the `EventBase` thread and using the `AlwaysEnqueue` variant, there is a race which can lead to termination.

Reviewed By: andriigrynenko

Differential Revision: D14114678

fbshipit-source-id: 9a0128d207f86ca34eb8a1d417766c095ed5e137
Summary: The reinit() method was removed but some comments mentioning it remained. Remove them.

Reviewed By: yfeldblum

Differential Revision: D14277614

fbshipit-source-id: 389ef3e2afe1fdc60f37aaec2f8a48dfa0b1436c
Summary: [Folly] Remove include of `glog` from `Range.h`.

Reviewed By: lbrandy, Orvid

Differential Revision: D14114164

fbshipit-source-id: e227609e9214ab39ff272e44519d34f3047fe025
Differential Revision:
D14114164

Original commit changeset: e227609e9214

fbshipit-source-id: 99d0fde58e512224915d7a00893ddf15c547dfba
Summary: [Folly] Support C++11 in `folly/IPAddress.h` w.r.t. `aligned_union` by avoiding `aligned_union_t`, which is C++14.

Reviewed By: spalamarchuk, mengz0

Differential Revision: D14278604

fbshipit-source-id: 474ac0aa0110f4939d1042d334b69feb6b2644cb
Summary:
[Folly] Mark `BitIterator<BaseIterator>` with the iterator category of `BitIterator`.

As one benefit, using `std::distance` to find the distance between two `BitIterator`s gotten from a random access container such as `std::vector` is now constant-time.

Fixes #1026.

Reviewed By: ot

Differential Revision: D14294852

fbshipit-source-id: 2345bc73dec169803ae41b9391687e89ad77207b
Summary: The property_set_insert_t<PS1, PS2> template metafunction had a bug that meant that it would fail to replace elements in the base set that had the same category as elements from the set being inserted.

Reviewed By: ericniebler, kirkshoop

Differential Revision: D14268145

fbshipit-source-id: ccc64e455bc6ff2073229e028a19d64dc7050092
Summary:
- On some platforms, `CMAKE_SYSTEM_ARCHITECTURE` may resolve to an empty
  string.
- When this value is used without quotes to the `string` command, it is
  ill-formed with error:

```
  CMake Error at CMakeLists.txt:174 (string):
  string sub-command FIND requires 3 or 4 parameters.
```

- Explicitly wrap the value to the `string` operation in quotes to treat
  `CMAKE_SYSTEM_ARCHITECTURE` as a string, which is allowed to be empty.
  The result is that the `string` operation will not result in a hard
  error.
Pull Request resolved: #1040

Reviewed By: calebmarchent

Differential Revision: D14293784

Pulled By: yfeldblum

fbshipit-source-id: cd5924fa62277aa07e930c7b088197ef596be977
@sandraiser sandraiser merged commit 6b329bb into sandraiser:master Mar 4, 2019
sandraiser pushed a commit that referenced this pull request Mar 4, 2019
Summary:
TSAN flagged the following lock inversion below. The reason for this lock inversion
is because of the following example usage:

1. set request context B, save old one A (lock B, lock A)
2. later on, reset request context back to A, get the old one back B (lock A, lock B)

In the two calls, we always locked the the new context first, resulting
in a lock inversion. The fix is to always acquire them in a consistent
order using `folly::acquireLocked`

TSAN report:

```
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=1574307)
  Cycle in lock order graph: M159 (0x7b1400000138) => M157 (0x7b14000000e8) => M159

  Mutex M157 acquired here while holding mutex M159 in main thread:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    facebook#8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    facebook#9 0x34916 in RequestContextScopeGuard ./folly/io/async/Request.h:269

  Mutex M159 previously acquired by the same thread here:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    facebook#8 0x36a7b in folly::RequestContext::create() ./folly/io/async/Request.h:119
    facebook#9 0x34916 in RequestContextScopeGuard ./folly/io/async/Request.h:269

  Mutex M159 acquired here while holding mutex M157 in main thread:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7530 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:243
    facebook#8 0x34b99 in ~RequestContextScopeGuard ./folly/io/async/Request.h:278

  Mutex M157 previously acquired by the same thread here:
    #0 0x48f18c in AnnotateRWLockAcquired ??:?
    #1 0x315495 in folly::SharedMutexImpl<false, void, std::atomic, false>::lock_shared() ./folly/SharedMutex.h:385
    facebook#2 0x1b0eb8 in folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:157
    facebook#3 0x1b0e68 in std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false> >(folly::SharedMutexImpl<false, void, std::atomic, false>&) ./folly/LockTraits.h:493
    facebook#4 0x1d63a9 in LockedPtrBase ./folly/Synchronized.h:1026
    facebook#5 0x1d6258 in LockedPtr ./folly/Synchronized.h:1326
    facebook#6 0x1c9a17 in folly::SynchronizedBase<folly::Synchronized<folly::RequestContext::State, folly::SharedMutexImpl<false, void, std::atomic, false> >, (folly::detail::MutexLevel)1>::rlock() const ./folly/Synchronized.h:123
    facebook#7 0x1c7516 in folly::RequestContext::setContext(std::shared_ptr<folly::RequestContext>) ./folly/io/async/Request.cpp:242
    facebook#8 0x34b99 in ~RequestContextScopeGuard ./folly/io/async/Request.h:278

```

Reviewed By: igorsugak

Differential Revision: D9797715

fbshipit-source-id: a0f991ae0ec8f3e9d33af6e05db49eceaf3a5174
sandraiser pushed a commit that referenced this pull request Mar 4, 2019
Summary:
This is mostly a POC. It will probably fail in some cases, but it's better than nothing.
Sample output:
  (gdb) co_bt this
  0x292d70 <zero()>
  0x293f50 <one()>
  0x295050 <two()>
  0x296150 <three()>
  0x297250 <folly::coro::TaskWithExecutor<int>::start() &&::{lambda(folly::Promise<int>, folly::coro::TaskWithExecutor<int>)#1}::operator()(folly::Promise<int>, folly::coro::TaskWithExecutor<int>) const>

Reviewed By: jwiepert

Differential Revision: D13727139

fbshipit-source-id: bff98eb4f5eb2ebd73c880d3b525172782f87511
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.