Injectable singleton class

[alyssawilk]

Description:
-Adds a ThreadSafe singleton class which can be accessed from all threads and overridden with an injected instance for custom implementations for test code or custom Envoy builds.
-Replaces OsSysCallsImpl being plumbed everywhere it is needed with a new ThreadSafe singleton as a sample use case (and to avoid having to plumb the OsSysCallsImpl to all users for test overrides)
-Adds abseil as a dependency

Fixes #1808
Step 1 of #1754

Risk Level: Low: mainly a refactor to how OsSysCallsImpl is accessed

Testing:
Existing OS tests continue to pass, now using an inject-able mock singleton
test/common/common/singleton_test.cc unit testing the new class

[htuch]

Nice approach, I like it.

[htuch]

It's a bit confusing to have the mutable thread safe singleton inherit from ConstSingleton. Is there a huge benefit here? Do we ever even want to mock out the stuff in ConstSingleton (it's mostly just literals/constant strings etc.)?

[jmillikin-stripe]

Nit: the upstream workspace name is com_google_absl. Bazel will warn if you use a different local name, and says this will eventually be an error.

[jmillikin-stripe]

The use of ABSL looks correct.

I'm slightly -1 on the existence of ThreadSafeSingleton<T>, because it encourages multiple types of singletons in the future -- that's a dark road to start down. Do you think it'd make sense to have just one "os" singleton, that has a "syscalls" accessor?

class OS {
 public:
  static OS& global_instance();
  OsSyscalls& syscalls();
 private:
  // ...
};

Another pattern I've seen used with success is a "global deps" class that has things that would be singletons. This is easier to plumb through parameters because it only has to be done once, and it avoids the use of mutable global state.

class OsSyscalls { ... };

class HasOsSyscalls {
 public:
  HasOsSyscalls(OsSyscalls&);
  OsSyscalls& syscalls();
};

void SomeFuncThatNeedsInjection(HasOsSyscalls& deps) {
  deps.syscalls().do_whatever();
}

class GlobalDeps : public HasOsSyscalls, public HasSomeOtherThing {
 public:
  // not static
  std::unique_ptr<GlobalDeps> New();
};

(note to @htuch: it would be awesome if some of the C++ server dependency stuff could be polished up enough for absl)

[htuch]

@jmillikin-stripe what do you mean by the C++ server dependency stuff?

[alyssawilk]

@jmillikin-stripe can you clarify why singletons are a dark road? I think having many types would be confusing but the existing singleton class is basiscally glorified static data not singletons as I'm used to using them.

If we want to have a global data registry akin to the current per-thread registry I don't think that's a bad way to go, but I'd at least like clarification on what else we think it gets us

[mattklein123]

I think @jmillikin-stripe is echoing my general feelings about these kinds of singletons: It can be difficult to reason about what is going on in code and tests. (FWIW this is why in general the code plumbs through all parameters. Much easier to mock and reason about without having to clean things up between tests).

If we want to special case OS syscalls that's fine, but we might not want to allow this in a general purpose sense. @jmillikin-stripe LMK if that is accurate.

[alyssawilk]

I can of course just stick the singleton-semantics here in the OsSysCalls but I'd be really surprised if we don't encounter another thing in this class. It sounds like asking around the factory registry is another thing we'd like to override-inject? I'd rather design a general purpose solution to an inject-able global object that one-off

I'm not clear on why singletons are harder to reason about in code than passing abstract APIs. If I could get more of an understanding of the objections there it'd be helpful since I find it easier to go direct to the Singleton rather than go up 5 levels of code to see how it was created.

Singletons persisting across tests is a more familiar argument. I've seen a mix of "use singletons and have UnsafeReset() semantics for when you want to guarantee fresh state" and "implementing [something that's a cross between singleton and thread-local-storage], say envoy-local-storage, where Envoy main would "register OS sys calls object if nothing has registered it first" etc. and then they're all torn down automatically with the Envoy instance so you don't have to worry about persistence. I think that'd address the persistence issue so if that's preferable let me know.

[mattklein123]

I'm not clear on why singletons are harder to reason about in code than passing abstract APIs. If I could get more of an understanding of the objections there it'd be helpful since I find it easier to go direct to the Singleton rather than go up 5 levels of code to see how it was created.

I think this is a matter of opinion/preference. In actuality I don't really feel that strongly about it but I do personally prefer to not have singletons floating all over the place that get injected. Happy to get outvoted.

say envoy-local-storage, where Envoy main would "register OS sys calls object if nothing has registered it first" etc. and then they're all torn down automatically with the Envoy instance so you don't have to worry about persistence. I think that'd address the persistence issue so if that's preferable let me know.

Personally I prefer something like this. This is kind of but not quite what this is: https://github.com/envoyproxy/envoy/blob/master/include/envoy/singleton/manager.h (The idea behind <- is to allow singletons that come and go).

In the end, like I said above, I don't feel that strongly about this and if this is the direction people want to go I'm happy to review as is.

[alyssawilk]

The singleton manager is sort of akin to the registry I had in mind except it has both the access problem of the current OsSystemCallbacks (needing to be plumbed through everywhere since it's not at base a singleton) and the annoyance problem of data registries (things created in a per-Envoy context needing to be manually created for every unit test which needs them).

I think the main objection here is the idea of feature creep of mutable global state singletons. I believe this particular singleton doesn't have that (scoped injectors, OS is technically affected but not in a way that will be annoying in test) so the least-unhappy-making solution would be to either fold the singleton logic into the OS specific class or put a big warning on the singleton class that it should not be used for anything stateful and in general it's worth filing an issue and getting design approval before using. I lean towards the latter, but I can go either way. It doesn't matter for us - we have a bunch of use cases where I know we'll use singletons internally but we can use the google3 singleton is functionally equivalent. I think it'd be nice if folks who are not singleton averse and don't have access to google3 could use the common utility in their own downstream repos but they can always roll their own.

[htuch]

I think we have good precedent for allowing things that aren't usually great style but are useful in narrow situations and which can be exercised with good taste, e.g. various points in https://github.com/envoyproxy/envoy/blob/master/STYLE.md#error-handling. This is ultimately a judgement call, we expect reviewers and developers to work to avoid creep. So, my vote is with the approach in this PR.

[mattklein123]

I'm fine proceeding with the review as is with potentially a comment warning / style guide update per @alyssawilk suggestion.

[mattklein123]

Sorry @alyssawilk one other request. With this change, we will have 3 singleton types:

1. const singleton
2. mutable singleton
3. singleton manager

Would it be worth it to move them all into the singleton namespace with a short explanation of what to use when?

[akonradi]

The factory registries are a fourth type of singleton, though they could be implemented in terms of this type of mutable singleton.

Tangentially, why do we have both mutable global-scope singletons and singletons scoped by singleton managers? Is the singleton manager's weak_ptr a benefit?

[mattklein123]

Tangentially, why do we have both mutable global-scope singletons and singletons scoped by singleton managers? Is the singleton manager's weak_ptr a benefit?

This was implemented for singletons that cannot be created until a certain point and should be freed when they are no longer needed. It could likely be implemented as a kind of global mutable singleton also, but as I said above, I'm not generally a fan of this pattern. If you want to reimplement that's fine.

[htuch]

LGTM

[htuch]

It did occur to me that there is some truth-in-advertising issue with a ThreadSafeSingleton that isn't thread safe but forces you to be thread safe. Anyway, potatoes potatahs.

[htuch]

Nit: double space after period.

[alyssawilk]

By design. Is there a style guide line item for this?

[htuch]

@dnoe also does this :) I think we agreed we would do single space, double space is some kind of crazy from the days of typewriters.

[htuch]

Not a huge fan of multithreaded unit tests with arbitrary interleavings. These will be hard to replay when tests flake. Would be a bit more deterministic if we created these on different threads and then ran the routines one per thread. For this particular case, I don't think it matter, it's not going to flake, but as a general principle for future.

[alyssawilk]

Can you rephrase the request here? I think to really test multithreadedness you have to have contention which you can't force without trying to do the updates all at once which means good multi-threaded tests should be non-deterministic by design.

[htuch]

The point is that tests have more limited value when they can't be repeated on flakes. I would prefer the ability to control the interleaving precisely of thread contention based on provided seed, but I don't think it's worth it for this simple example.

[htuch]

Looks like there is some merge to do. GTG after that.

[mattklein123]

LGTM also modulo merge

[jmillikin-stripe]

Minor nitpick: Abseil is a native Bazel project, so we should use its declared workspace name here. Otherwise you'll get a warning at build time.

https://github.com/abseil/abseil-cpp/blob/master/WORKSPACE

_repository_impl("com_google_absl")