server: Contain the Configuration::MainImpl directly in server objects. #5184

jmarantz · 2018-12-02T17:18:44Z

Description: I bumped into this in #4997 due to clang-tidy and pulled it to this separate PR, as it's kind of outside the scope of #4997. I think this is the simplest option for making this code clang-tidy compliant.

Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

…cases. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

venilnoronha

I'd prefer to keep the std::unique_ptr<Configuration::Main> pointer instead of referring to an implementation in the headers. That said, we could still simplify its initialization by replacing the reset calls with make_unique.

jmarantz · 2018-12-02T17:49:54Z

@venilnoronha as I said I'm not wedded to this but it has a PRO: less lines of code, and less conditional. What's the reason for your preferring it as is? We should probably leave a comment explaining the rationale.

venilnoronha · 2018-12-02T17:58:27Z

The reason is that the header is now adding a dependency on an implementation rather than just an abstraction.

jmarantz · 2018-12-02T18:22:44Z

So your concern is mainly about compile-speed pulling in the extra header? That makes sense, though server.h is dependent on lots of other other impls so I'm not sure what's special about this one, and it would be good to converge on style preferences in https://github.com/envoyproxy/envoy/blob/master/STYLE.md.

I do like the idea of making improvements to compile-speed (see #5182).

I guess to have the object guaranteed present (to avoid conditionals) we'd have to have two variables in the class

  std::unique_ptr<Configuration> config_storage_;
  Configuration& config_;

and then init them like:

  : config_storage_(std::make_unique<....>(...)),
    config_(*config_storage_), ...

but applying this broadly would get boring. Maybe we should have some kind of OwnedReference<T> template wrapper to encapsulate the 2 variables and their initialization. WDYT?

venilnoronha · 2018-12-02T18:51:59Z

It's both design and performance. I did see the references to other headers, and I think, we should address those in the future.

Regarding your point on avoiding conditionals, I like the OwnedReference<T> idea; however, it boils down to performance v/s memory. I'd personally pick memory optimization over performance in this case i.e. when considering the larger system.

jmarantz · 2018-12-02T20:56:58Z

OK, I am back to wanting to push this PR forward as is, despite the fact that it adds a dependency. That's because the current state of the code is not clang-tidy-clean and this edit is the simplest way to make it clean.

See #4997 (comment) for more details.

jmarantz · 2018-12-02T20:59:43Z

That comment link doesn't take you to the right spot in #4997 -- here's the relevant part of the comment:

OK I remember why I messed with this now. clang-tidy doesn't like it the way the code is in the repo now. Because it grandfathers in old non-compliant code it was OK until I made edits near it, and my previous state (now reflected in #5184) makes it clang-tidy clean.

Also it's super-annoying to iterate with clang-tidy as it takes 40 minutes to run locally for me under docker, and it's non-deterministic in CI.

Here's the conundrum:

clang-tidy doesn't like naked calls to new, with Config* variables.
the semantics of the code as is require two pointers to the same memory: one owned
by Server that's got the interface type, and one local that has the Impl type. The Impl
type provides an initialize() method and the interface does not.
The implementation of the configure method requires that server->config_ be populated
Without a more significant refactoring I don't think it's possible to make this code work and also make clang-tidy happy.

jmarantz · 2018-12-02T21:06:29Z

Here's a pattern that (I think) makes clang-tidy happy (will know in 40m) and also passes tests:

  auto cfg = std::make_unique<Configuration::MainImpl>();
  Configuration::MainImpl* main_config = cfg.get();
  config_ = std::move(cfg);
  main_config->initialize(bootstrap, *this, *cluster_manager_factory_);

I think the solution offered in this PR is better though.

mattklein123

I understand your concerns about clang-tidy, but I think the change introduces bugs and it feels like change for change sake. I haven't read your previous complete conversation but is there some other way to make clang-tidy happy here without these changes?

/wait

mattklein123 · 2018-12-02T22:12:13Z

source/server/config_validation/server.cc

@@ -106,8 +100,8 @@ void ValidationInstance::shutdown() {
  // do an abbreviated shutdown here since there's less to clean up -- for example, no workers to
  // exit.
  thread_local_.shutdownGlobalThreading();
-  if (config_ != nullptr && config_->clusterManager() != nullptr) {
-    config_->clusterManager()->shutdown();
+  if (config_.clusterManager() != nullptr) {


As I already mentioned in the other PR, this is almost certainly not correct. The nullptr check was present previously because shutdown() can be called before there is a config. The cluster manager is created during initialize(). I don't think there is a guarantee that the cluster manager exists here. Same applies in the non-validation server I think (or for sure that one, maybe not this one).

jmarantz · 2018-12-02T23:15:39Z

The nullptr check you are looking for is not necessary in this PR because the member var is no longer a pointer.

We are still checking that the cluster manager is non-null.

Imo this is the cleanest option but in the comment above I gave a different one that sticks with an interface pointer rather than an impl value.

mattklein123 · 2018-12-03T00:26:14Z

We are still checking that the cluster manager is non-null.

Sorry, I clearly did not read this carefully enough. Yes, this is fine. OK I agree this is a good cleanup and it's worth it to have the impl inline. If we want to have tests that use the interface later we can revisit then.

mattklein123

LGTM with small comment. Also needs master merge.

/wait

mattklein123 · 2018-12-03T00:27:24Z

source/server/server.cc

-  config_.reset(main_config);
-  main_config->initialize(bootstrap_, *this, *cluster_manager_factory_);
+  // Now the configuration gets parsed. The configuration may start setting
+  // thread local data per above. See MainImpl::initialize() for why ConfigImpl


The comment here is not quite accurate anymore, since we are not initializing a pointer which gets called through. We still need to call initialize() though after we have the factory setup. Can you fix up the comment inside MainImpl::initialize()?

I think the comment in configuraiton_impl.h at the declaration for initialize() is still accurate:

* Initialize the configuration. This happens here vs. the constructor because the initialization * will call through the server into the config to get the cluster manager so the config object * must be created already.

In this PR the language about 'pointer dance' here in this file has been changed.

Pedantically, I think it's a little different. Previously, since we were using a pointer, we needed to create the object then call initialize so the pointer is valid. Now, we create the config when we create the server, but we don't initialize it until we have the various pieces (factory) that we need. Personally I would alter to make it more clear but up to you.

I gave it another shot; ptal. I'm not sure if I've adequately captured your concern.

* MainImpl is created in two phases. In the first phase it is * default-constructed without a configuration as part of the server. The * server won't be fully populated yet. initialize() applies the * configuration in the second phase, as it requires a fully populated server.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz

master merge done.

jmarantz · 2018-12-03T02:39:31Z

source/server/server.cc

-  config_.reset(main_config);
-  main_config->initialize(bootstrap_, *this, *cluster_manager_factory_);
+  // Now the configuration gets parsed. The configuration may start setting
+  // thread local data per above. See MainImpl::initialize() for why ConfigImpl


I think the comment in configuraiton_impl.h at the declaration for initialize() is still accurate:

* Initialize the configuration. This happens here vs. the constructor because the initialization * will call through the server into the config to get the cluster manager so the config object * must be created already.

In this PR the language about 'pointer dance' here in this file has been changed.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-12-03T13:37:32Z

/retest

repokitteh-read-only · 2018-12-03T13:37:36Z

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #5184 (comment) was created by @jmarantz.

see: more, trace.

mattklein123

LGTM

/wait

source/server/configuration_impl.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-12-03T18:53:17Z

/retest

repokitteh-read-only · 2018-12-03T18:53:21Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5184 (comment) was created by @jmarantz.

see: more, trace.

…s. (envoyproxy#5184) * Contain the Configuration::MainImpl directly in server objects, simplifying code and making it clang-tidy compliant. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz added 2 commits December 2, 2018 11:50

Contain ConfigurationImpl directly in the servers, simplifying a few …

72e6dd3

…cases. Signed-off-by: Joshua Marantz <jmarantz@google.com>

all tests working.

01bd77a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

venilnoronha reviewed Dec 2, 2018

View reviewed changes

jmarantz mentioned this pull request Dec 2, 2018

stats: Capture all the strings used to accumulate http stats in an object, plumbed through the system. #4997

Merged

mattklein123 requested changes Dec 2, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 2, 2018

mattklein123 self-assigned this Dec 2, 2018

mattklein123 requested changes Dec 3, 2018

View reviewed changes

Merge branch 'master' into config-impl-contained

7802653

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Dec 3, 2018

View reviewed changes

repokitteh-read-only bot removed the waiting label Dec 3, 2018

Reword comment.

c3508fa

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Dec 3, 2018

View reviewed changes

source/server/configuration_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Dec 3, 2018

Remove stray partial comment.

80fa70b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Dec 3, 2018

mattklein123 approved these changes Dec 3, 2018

View reviewed changes

jmarantz merged commit 90bd816 into envoyproxy:master Dec 3, 2018

jmarantz deleted the config-impl-contained branch December 3, 2018 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Contain the Configuration::MainImpl directly in server objects. #5184

server: Contain the Configuration::MainImpl directly in server objects. #5184

jmarantz commented Dec 2, 2018 •

edited

Loading

venilnoronha left a comment

jmarantz commented Dec 2, 2018

venilnoronha commented Dec 2, 2018

jmarantz commented Dec 2, 2018 •

edited

Loading

venilnoronha commented Dec 2, 2018

jmarantz commented Dec 2, 2018 •

edited

Loading

jmarantz commented Dec 2, 2018

jmarantz commented Dec 2, 2018 •

edited

Loading

mattklein123 left a comment

mattklein123 Dec 2, 2018

jmarantz commented Dec 2, 2018

mattklein123 commented Dec 3, 2018

mattklein123 left a comment

mattklein123 Dec 3, 2018

jmarantz Dec 3, 2018

mattklein123 Dec 3, 2018

jmarantz Dec 3, 2018

jmarantz left a comment

jmarantz Dec 3, 2018

jmarantz commented Dec 3, 2018

repokitteh-read-only bot commented Dec 3, 2018

mattklein123 left a comment

jmarantz commented Dec 3, 2018

repokitteh-read-only bot commented Dec 3, 2018

server: Contain the Configuration::MainImpl directly in server objects. #5184

server: Contain the Configuration::MainImpl directly in server objects. #5184

Conversation

jmarantz commented Dec 2, 2018 • edited Loading

venilnoronha left a comment

Choose a reason for hiding this comment

jmarantz commented Dec 2, 2018

venilnoronha commented Dec 2, 2018

jmarantz commented Dec 2, 2018 • edited Loading

venilnoronha commented Dec 2, 2018

jmarantz commented Dec 2, 2018 • edited Loading

jmarantz commented Dec 2, 2018

jmarantz commented Dec 2, 2018 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Dec 2, 2018

Choose a reason for hiding this comment

jmarantz commented Dec 2, 2018

mattklein123 commented Dec 3, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Dec 3, 2018

Choose a reason for hiding this comment

jmarantz Dec 3, 2018

Choose a reason for hiding this comment

mattklein123 Dec 3, 2018

Choose a reason for hiding this comment

jmarantz Dec 3, 2018

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Dec 3, 2018

Choose a reason for hiding this comment

jmarantz commented Dec 3, 2018

repokitteh-read-only bot commented Dec 3, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz commented Dec 3, 2018

repokitteh-read-only bot commented Dec 3, 2018

jmarantz commented Dec 2, 2018 •

edited

Loading

jmarantz commented Dec 2, 2018 •

edited

Loading

jmarantz commented Dec 2, 2018 •

edited

Loading

jmarantz commented Dec 2, 2018 •

edited

Loading