thread: add Windows implementation #5072

sesmith177 · 2018-11-16T22:51:00Z

Signed-off-by: Sam Smith sesmith177@gmail.com

Description:
Adds a Win32 implementation of the ThreadImpl class

Risk Level:
Low
Testing:
bazel build //source/... && bazel test //test/.... Also ran the tests on our Windows fork
Docs Changes:
N/A
Release Notes:
N/A

zyfjeff · 2018-11-17T02:42:00Z

Why not use std::thread?

mattklein123 · 2018-11-18T20:01:08Z

Why not use std::thread?

Historically, std::thread eats exceptions and prints useless crash/abort messages.

mattklein123 · 2018-11-18T20:01:40Z

@jmarantz @curiouserrandy can you take a look to see how this might fit in with the thread factory stuff? Not sure if it matters or not.

jmarantz · 2018-11-18T20:20:55Z

@eziskind is working on the thread factory.

jmarantz · 2018-11-18T20:22:26Z

@sesmith177 can you PTAL at #5055 ? It probably doesn't make sense to have both of these in flight at once.

curiouserrandy · 2018-11-18T22:16:46Z

I don't think I have relevant expertise on this PR. I'll unassign myself (if I can figure out how); please feel free to assign me or ping me if there's some reason I should be included.

sesmith177 · 2018-11-19T14:32:08Z

@jmarantz not a problem, we'll wait for that to be merged and then rework this PR

jmarantz · 2018-11-19T22:43:24Z

This looks good so far; might get a little simpler if you are adjusting only the implementation of ThreadSystem; hopefully #5055 will get merged soon.

For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in #4980, ThreadSystem in #5055 and #5072 and FileSystem in #5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-11-21T22:41:09Z

#5055 is now merged; and might make this change to support an alternate thread-system much more surgical.

Check out Thread::threadFactoryForTest() in test/test_common/utility.cc which you could probably just ifdef.

And check out the origin of the production thread-factory in source/exe/main_common.cc in the main_common() function. You could make your windows binary share MainCommonBase, but split off the main_common() function to another file which is not compiled in windows, and instantiate an alternate ThreadFactory in your new main().

yuval-k · 2018-11-23T18:16:38Z

source/common/thread/win32/thread_impl.cc

+
+ThreadImpl::ThreadImpl(std::function<void()> thread_routine) : thread_routine_(thread_routine) {
+  RELEASE_ASSERT(Logger::Registry::initialized(), "");
+  thread_handle_ = ::CreateThread(NULL, 0,


drive-by question: should this be _beginthreadex? (from the docs: A thread in an executable that calls the C run-time library (CRT) should use the _beginthreadex )

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-11-28T20:10:29Z

@jmarantz I've updated this PR to take into account #5055 and to touch fewer files

@yuval-k You're right -- thanks for catching that

eziskind · 2018-11-28T20:22:30Z

include/envoy/thread/thread.h

@@ -18,6 +37,8 @@ class Thread {
   * Join on thread exit.
   */
  virtual void join() PURE;
+
+  virtual ThreadHandle handle() const PURE;


Do we need to expose the thread handle? I don't see this method getting called anywhere.

We have some code in our fork (specifically the Windows version of Filesystem::WatcherImpl) that needs access to the thread handle. We haven't PR'd those changes back upstream yet but plan to eventually

For this PR can we remove the handle, as it's not used yet?

Then in the next PR we can discuss the handle and its usage in FilesystemImpl. I predict that I'm would suggest you make a windows-specific subclass of that too, but can't tell until I see it :)

In general I hope we can approach portability with injection of alternate class implementations rather than ifdefs, particularly ifdefs in an interface header.

Not a problem.

The specific usage is our workaround for the fact that libevent can only watch sockets on Windows (as opposed to arbitrary). But we'll get to that in a future PR

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz · 2018-11-29T16:02:12Z

source/common/common/win32/thread_impl.cc

+namespace Envoy {
+namespace Thread {
+
+ThreadImpl::ThreadImpl(std::function<void()> thread_routine) : thread_routine_(thread_routine) {


It looks like this is the exact same name as the posix one, so the decision is made at link-time, and if you link both files in you'd get a duplicate symbol conflict.

That's not really a concern for windows vs posix, but you may also want an alternate thread system even if more than one is possible on the platform. That's something that we need (google has some custom thread infrastructure).

I'll give you another example: you may want a stats-collecting thread-factory decorator that keeps a gauge of the active thread-count or other metrics. I'd keep that separate from the platform-specific impls, so (e.g.) you could use that to decorate either windows or posix one.

Can we inject the right platform-specific ThreadFactory where needed rather than having this be a link-time decision?

sure, we broke out separate main.cc files for Windows / POSIX and inject the correct thread factory there

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz

Looking a lot better; left a few more comments.

jmarantz · 2018-11-29T22:32:59Z

source/exe/main_common_base.cc

-}
-
-} // namespace Envoy
+} // namespace Envoy


add newline

jmarantz · 2018-11-29T22:35:48Z

test/test_common/utility.cc

@@ -347,7 +347,11 @@ MockedTestAllocator::~MockedTestAllocator() {}
 namespace Thread {

 ThreadFactory& threadFactoryForTest() {
-  static ThreadFactoryImpl* thread_factory = new ThreadFactoryImpl();
+#if !defined(WIN32)


flip if/else and invert the conditional

So far, we've been following this pattern (if !defined(WIN32)) in our PRs. We don't have a strong opinion on which way the conditional should be written, but do think we should be consistent about which way we pick.

There's about ~8 other places we'd have to change as well, all but 1 in test code. If we want to make the change as part of this PR, that's fine with us.

source/exe/win32/main.cc

jmarantz · 2018-11-29T22:39:45Z

include/envoy/thread/thread.h

 namespace Envoy {
 namespace Thread {

+#if !defined(WIN32)
+using ThreadId = int32_t;


We can avoid ifdefs in the interface here and instead have ThreadId be an abstract base class, which I'd give two or three methods:
-- compare to another ThreadID
-- determine if it's the current thread
-- serialize to a string for debug purposes

in the past I've done this this way: https://github.com/apache/incubator-pagespeed-mod/blob/20fbc5575c8fc825a137f6f1b2015829a9768f81/pagespeed/kernel/base/thread_system.h#L117

Note referenced there: https://linux.die.net/man/3/pthread_self

POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted. Therefore, variables of type pthread_t can't portably be compared using the C equality operator (==); use pthread_equal(3) instead.

jmarantz · 2018-11-29T22:42:31Z

source/common/common/posix/thread_impl.cc

+#elif defined(__APPLE__)
+  uint64_t tid;
+  pthread_threadid_np(NULL, &tid);
+  return static_cast<int32_t>(tid);


this is probably pre-existing, but IMO further evidence that I think we want an abstract base class for ThreadID.

I could see also a subclass for Apple and a subclass for Linux I guess.

Yep, this is pre-existing. We will probably hold off on splitting Apple from Linux, as we don't really have a set up for testing on OSX

agreed; no need to add complexity here.

jmarantz · 2018-11-29T22:44:31Z

source/common/common/posix/thread_impl.h

+ */
+class ThreadImplPosix : public Thread {
+public:
+  ThreadImplPosix(std::function<void()> thread_routine);


FWIW I suspect you don't need ThreadImplPosix to be exposed in the header -- it can be private to the ThreadFactoryImplPosix impl and in the cc file. This is not a big deal but I feel like it's nice to minimize what has to go into h files, for compile-speed and for human understanding of interfaces.

I agree that we don't need ThreadImplPosix exposed in the header, but we do need ThreadImplWin32 exposed (so one of our Windows-specific classes can access data we don't want to put on the interface). It may be more consistent to have both exposed rather than one exposed and one placed in the cc file

OK leave it exposed then, this is not a big deal, and if we do subclass Apple vs Linux in the future we'll need the shared posix impl exposed at least as a class declaration.

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz · 2018-11-30T15:17:04Z

ping when ready for another round.

Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-12-03T19:14:20Z

@jmarantz I think we're good to go for another round.

jmarantz · 2018-12-14T17:31:42Z

test/test_common/utility.cc

-  const int rc = ::symlink(target.c_str(), link.c_str());
-  ASSERT_EQ(rc, 0);
-#else
+#if defined(WIN32)


I've seen you use _MSC_VER elsewhere and WIN32 here. Why's that? If possible can we use just one condition?

Also why #if defined(WIN32) rather than the more concise #ifdef WIN32?

Usually I see the define(xxx) form in the context of a more complex logical expression: if defined(WIN32) || defined(APPLE)

_MSC_VER is automatically defined if the compiler is MSVC, while we manually define WIN32 in our Bazel setup. The reason for the distinction is that some changes (e.g. the PACKED_STRUCT definition) depend on the specific compiler being used (MSVC vs. mingw vs. clang, etc.), while others depend on the specific platform (Windows vs. Linux, etc.)

Of course, right now, we only plan on doing the work to build Envoy with MSVC on Windows, but we could imagine a situation where someone wanted to build Envoy on Windows with a different compiler.

As for #if defined vs #ifdef, no specific reason

The _MSC_VER vs WIN32 came from feedback on a previous PR: #4645 (comment)

jmarantz · 2018-12-14T17:33:30Z

test/test_common/utility.cc

  // use MoveFileEx, since ::rename will not overwrite an existing file. See
  // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017
  const BOOL rc = ::MoveFileEx(old_name.c_str(), new_name.c_str(), MOVEFILE_REPLACE_EXISTING);
  ASSERT_NE(0, rc);
+#else
+  const int rc = ::rename(old_name.c_str(), new_name.c_str());


I think all this should be inside the filesystem abstraction (include/envoy/filesystem/filesystem.h). Can you at least add a TODO for now?

Note that the Filesystem and ThreadFactory are both part of Api::Api so you can see the convergence of all this.

We have this broken out here since we didn't want to add production code that is only called from tests. If moving this code into the filesystem abstraction is the way to go, we will do that as a part of the filesystem PR that we are currently working on.

That's a good point. Leaving these in the test-utility makes sense as long as the functionality is not needed in production.

jmarantz · 2018-12-14T17:37:22Z

source/exe/main_common.cc

    : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info),
      base_(options_, real_time_system_, default_test_hooks_, prod_component_factory_,
-            std::make_unique<Runtime::RandomGeneratorImpl>(), thread_factory_) {}
+            std::make_unique<Runtime::RandomGeneratorImpl>(), platform_main.threadFactory()) {}


I think this fits into createApiForTest but may require some auditing. The caller of that should ensure the returned Api::ApiPtr lives as long as needed.

Or is it a problem of some sort to have the ctor/dtor called on every unit test within a single process?

jmarantz · 2018-12-14T17:54:28Z

source/exe/posix/platform_main.h

+#ifndef __APPLE__
+    // absl::Symbolize mostly works without this, but this improves corner case
+    // handling, such as running in a chroot jail.
+    absl::InitializeSymbolizer(argv[0]);


should we have posix and windows implementions inherit from a common base-class that calls the symbolizer?

...alternatively, you could just leave the call to InitializeSymbolizer in main() where it is now, and then you don't need to pass in argc,argv to PlatformMain.

jmarantz · 2018-12-14T18:00:20Z

All my above suggeestions are for TODOs in this PR; the only thing I'd like to see changed in this PR is consistent use of a #ifdef WIN32 if possible.

sesmith177 · 2018-12-14T18:40:13Z

/retest

repokitteh-read-only · 2018-12-14T18:40:16Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5072 (comment) was created by @sesmith177.

see: more, trace.

sesmith177 · 2018-12-14T19:48:29Z

We will replace #if defined(XXX) with #ifdef XXX where appropriate

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz

Looks great; just a couple of nits left. Let's get this done.

jmarantz · 2018-12-16T21:57:49Z

source/common/common/posix/thread_impl.h

+ */
+class ThreadFactoryImplPosix : public ThreadFactory {
+public:
+  ThreadFactoryImplPosix() {}


omit empty ctor.

jmarantz · 2018-12-16T22:15:44Z

source/exe/posix/platform_main.h

+#ifndef __APPLE__
+    // absl::Symbolize mostly works without this, but this improves corner case
+    // handling, such as running in a chroot jail.
+    absl::InitializeSymbolizer(argv[0]);


...alternatively, you could just leave the call to InitializeSymbolizer in main() where it is now, and then you don't need to pass in argc,argv to PlatformMain.

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-12-17T13:32:13Z

@jmarantz added changes as requested

jmarantz · 2018-12-17T22:40:15Z

Hi -- I'm a relatively new maintainer and don't totally understand the flow yet. It looks like I can't merge this PR because there are still requested changes (although @htuch has LGTM'd so I think I should be able to merge). See #5072 (review)

It looks like you've addressed @htuch's suggestions but maybe you didn't click 'resolve conversation' for each comment?

sesmith177 · 2018-12-18T00:27:31Z

I think (though I don't know for sure) @htuch may need to explicitly approve the changes as well

htuch · 2018-12-18T03:21:10Z

@sesmith177 @jmarantz I'll take another pass now and merge if it looks all good!

htuch · 2018-12-18T03:32:22Z

source/exe/posix/platform_main.h

+
+namespace Envoy {
+
+class PlatformMain {


I'm still uncomfortable with this naming. PlatformMain to me should be a method, it's essentially an entry point, not a noun. Can we just make this PlatformImpl?

LGTM modulo this bikeshed :)

lizan

LGTM, one nit.

@jmarantz see https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#pr-review-policy-for-maintainers so I think we need @htuch's approval :)

lizan · 2018-12-18T03:00:38Z

source/common/api/api_impl.cc

@@ -16,7 +16,7 @@ Impl::Impl(std::chrono::milliseconds file_flush_interval_msec,
      file_system_(file_flush_interval_msec, thread_factory, stats_store) {}

 Event::DispatcherPtr Impl::allocateDispatcher(Event::TimeSystem& time_system) {
-  return Event::DispatcherPtr{new Event::DispatcherImpl(time_system)};
+  return Event::DispatcherPtr{new Event::DispatcherImpl(time_system, *this)};


nit: make_unique

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-12-18T13:41:06Z

@htuch @lizan I've addressed the feedback

htuch

Awesome, thanks!

htuch · 2018-12-19T03:37:45Z

Needs master merge @sesmith177, will merge as soon as that happens.

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Also, removes thread safety assertions due to changes introduced in (envoyproxy#5072); it is no longer possible to statically call functions to obtain/check the current thread ID. I will reintroduce these checks once static accessors are available again. Signed-off-by: Andres Guedez <aguedez@google.com>

For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in envoyproxy#4980, ThreadSystem in envoyproxy#5055 and envoyproxy#5072 and FileSystem in envoyproxy#5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Adds a Win32 implementation of the ThreadImpl class Risk Level: Low Testing: bazel build //source/... && bazel test //test/.... Also ran the tests on our Windows fork Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

sesmith177 requested review from htuch and lizan as code owners November 16, 2018 22:51

mattklein123 assigned jmarantz Nov 18, 2018

mattklein123 assigned curiouserrandy Nov 18, 2018

jmarantz mentioned this pull request Nov 19, 2018

test: Add a mechanism for creating singletons in tests #5086

Merged

yuval-k reviewed Nov 23, 2018

View reviewed changes

thread: add Windows implementation

944dbea

Signed-off-by: Sam Smith <sesmith177@gmail.com>

eziskind reviewed Nov 28, 2018

View reviewed changes

sesmith177 and others added 2 commits November 28, 2018 16:19

Don't expose thread handle

d5b659a

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Fix clang-tidy

da5e073

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Nov 29, 2018

View reviewed changes

Inject appropriate thread factory in main

779530f

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Nov 29, 2018

View reviewed changes

sesmith177 added 3 commits November 30, 2018 08:35

Flip #ifdef

c4c2c4e

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Also flip _MSC_VER

f78bdb9

Signed-off-by: Sam Smith <sesmith177@gmail.com>

Only pass arguments to MainCommonBase ctor that can change

2f95ad6

Signed-off-by: Sam Smith <sesmith177@gmail.com>

use abstract base class for threadid

c6b0bec

Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Dec 14, 2018

View reviewed changes

natalieparellano and others added 2 commits December 14, 2018 14:55

prefer #ifdef to #if defined()

d37414b

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

add todo

d7e09c5

Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz reviewed Dec 16, 2018

View reviewed changes

remove empty ctor + move symbolize

7d0acef

Signed-off-by: Sam Smith <sesmith177@gmail.com>

jmarantz previously approved these changes Dec 17, 2018

View reviewed changes

htuch suggested changes Dec 18, 2018

View reviewed changes

lizan reviewed Dec 18, 2018

View reviewed changes

fix nits

6313b4f

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 dismissed jmarantz’s stale review via 6313b4f December 18, 2018 13:38

htuch previously approved these changes Dec 19, 2018

View reviewed changes

Merge branch 'master' into add-windows-threads

f6cb1ee

Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 dismissed htuch’s stale review via f6cb1ee December 19, 2018 13:38

htuch merged commit 02b88e7 into envoyproxy:master Dec 19, 2018

AndresGuedez mentioned this pull request Dec 21, 2018

Thread safety assertion complications after #5072 #5390

Closed

sesmith177 mentioned this pull request Jan 3, 2019

filesystem: add Windows implementation #5470

Closed

thread: add Windows implementation #5072

thread: add Windows implementation #5072

Conversation

sesmith177 commented Nov 16, 2018

zyfjeff commented Nov 17, 2018

mattklein123 commented Nov 18, 2018

mattklein123 commented Nov 18, 2018

jmarantz commented Nov 18, 2018

jmarantz commented Nov 18, 2018

curiouserrandy commented Nov 18, 2018

sesmith177 commented Nov 19, 2018

jmarantz commented Nov 19, 2018

jmarantz commented Nov 21, 2018

Choose a reason for hiding this comment

sesmith177 commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

jmarantz commented Nov 30, 2018

sesmith177 commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Dec 14, 2018

sesmith177 commented Dec 14, 2018

repokitteh-read-only bot commented Dec 14, 2018

sesmith177 commented Dec 14, 2018

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sesmith177 commented Dec 17, 2018

jmarantz commented Dec 17, 2018

sesmith177 commented Dec 18, 2018

htuch commented Dec 18, 2018

Choose a reason for hiding this comment

htuch Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sesmith177 commented Dec 18, 2018

htuch left a comment

Choose a reason for hiding this comment

htuch commented Dec 19, 2018

jmarantz Nov 29, 2018 •

edited

Loading

jmarantz Nov 30, 2018 •

edited

Loading

htuch Dec 18, 2018 •

edited

Loading