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

thread: add Windows implementation #5072

Merged
merged 30 commits into from
Dec 19, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
944dbea
thread: add Windows implementation
sesmith177 Nov 28, 2018
d5b659a
Don't expose thread handle
sesmith177 Nov 28, 2018
da5e073
Fix clang-tidy
mhoran Nov 29, 2018
779530f
Inject appropriate thread factory in main
mhoran Nov 29, 2018
c4c2c4e
Flip #ifdef
sesmith177 Nov 30, 2018
f78bdb9
Also flip _MSC_VER
sesmith177 Nov 30, 2018
2f95ad6
Only pass arguments to MainCommonBase ctor that can change
sesmith177 Nov 30, 2018
c6b0bec
use abstract base class for threadid
achasveachas Dec 3, 2018
2da255b
Fix clang-tidy
achasveachas Dec 4, 2018
1138a93
remove operator== and api threadId/createThread wrappers
sesmith177 Dec 5, 2018
fdfb693
Revert "Only pass arguments to MainCommonBase ctor that can change"
natalieparellano Dec 5, 2018
0b13893
Merge branch 'master' into add-windows-threads
natalieparellano Dec 5, 2018
428dd06
Pass ThreadIdPtr to createWatchdog
natalieparellano Dec 5, 2018
91c440f
cleanup
natalieparellano Dec 5, 2018
37c1832
Simplify api mock
sesmith177 Dec 5, 2018
cca7795
Fix nits
sesmith177 Dec 10, 2018
c7a1233
Fix asan/tsan
sesmith177 Dec 10, 2018
3d8d124
Add PlatformMain to handle platform specific setup
sesmith177 Dec 11, 2018
0d230f2
fix osx build + upcast tid to int64
sesmith177 Dec 12, 2018
b0a5ab8
always include pthread.h
sesmith177 Dec 12, 2018
5b5cf22
fix nits
sesmith177 Dec 13, 2018
e696f0d
remove legacy main
sesmith177 Dec 14, 2018
a901bc0
Merge branch 'master' into add-windows-threads
sesmith177 Dec 14, 2018
f49b24f
fix check_format
sesmith177 Dec 14, 2018
70024c6
PlatformMain is member of MainCommon
sesmith177 Dec 14, 2018
d37414b
prefer #ifdef to #if defined()
natalieparellano Dec 14, 2018
d7e09c5
add todo
sesmith177 Dec 14, 2018
7d0acef
remove empty ctor + move symbolize
sesmith177 Dec 17, 2018
6313b4f
fix nits
sesmith177 Dec 18, 2018
f6cb1ee
Merge branch 'master' into add-windows-threads
sesmith177 Dec 19, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion include/envoy/event/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ envoy_cc_library(
hdrs = ["timer.h"],
deps = [
"//include/envoy/common:time_interface",
"//source/common/common:thread_lib",
"//source/common/event:libevent_lib",
],
)
1 change: 0 additions & 1 deletion include/envoy/event/timer.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "envoy/common/pure.h"
#include "envoy/common/time.h"

#include "common/common/thread.h"
#include "common/event/libevent.h"

namespace Envoy {
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ envoy_cc_library(
deps = [
"//include/envoy/server:watchdog_interface",
"//include/envoy/stats:stats_interface",
"//include/envoy/thread:thread_interface",
],
)

Expand Down Expand Up @@ -134,6 +135,7 @@ envoy_cc_library(
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/network:address_interface",
"//include/envoy/thread:thread_interface",
],
)

Expand Down
3 changes: 2 additions & 1 deletion include/envoy/server/guarddog.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "envoy/common/pure.h"
#include "envoy/server/watchdog.h"
#include "envoy/thread/thread.h"

namespace Envoy {
namespace Server {
Expand All @@ -28,7 +29,7 @@ class GuardDog {
*
* @param thread_id A numeric thread ID, like from Thread::currentThreadId()
*/
virtual WatchDogSharedPtr createWatchDog(int32_t thread_id) PURE;
virtual WatchDogSharedPtr createWatchDog(Thread::ThreadId thread_id) PURE;

/**
* Tell the GuardDog to forget about this WatchDog.
Expand Down
3 changes: 2 additions & 1 deletion include/envoy/server/watchdog.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "envoy/common/pure.h"
#include "envoy/event/dispatcher.h"
#include "envoy/thread/thread.h"

namespace Envoy {
namespace Server {
Expand Down Expand Up @@ -35,7 +36,7 @@ class WatchDog {
* This can be used if this is later used on a thread where there is no dispatcher.
*/
virtual void touch() PURE;
virtual int32_t threadId() const PURE;
virtual Thread::ThreadId threadId() const PURE;
virtual MonotonicTime lastTouchTime() const PURE;
};

Expand Down
21 changes: 21 additions & 0 deletions include/envoy/thread/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,28 @@

#include "common/common/thread_annotations.h"

#ifdef __linux__
#include <sys/syscall.h>
#elif defined(__APPLE__)
#include <pthread.h>
#elif defined(WIN32)
#include <Windows.h>
// <windows.h> defines some macros that interfere with our code, so undef them
#undef DELETE
#undef GetMessage
#endif

namespace Envoy {
namespace Thread {

#if !defined(WIN32)
typedef int32_t ThreadId;
typedef pthread_t ThreadHandle;
#else
typedef DWORD ThreadId;
typedef HANDLE ThreadHandle;
#endif

class Thread {
public:
virtual ~Thread() {}
Expand All @@ -18,6 +37,8 @@ class Thread {
* Join on thread exit.
*/
virtual void join() PURE;

virtual ThreadHandle handle() const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

typedef std::unique_ptr<Thread> ThreadPtr;
Expand Down
23 changes: 22 additions & 1 deletion source/common/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ load(
"//bazel:envoy_build_system.bzl",
"envoy_basic_cc_library",
"envoy_cc_library",
"envoy_cc_platform_dep",
"envoy_cc_posix_library",
"envoy_cc_win32_library",
"envoy_include_prefix",
"envoy_package",
)
Expand Down Expand Up @@ -182,9 +185,27 @@ envoy_cc_library(
srcs = ["thread.cc"],
hdrs = ["thread.h"],
external_deps = ["abseil_synchronization"],
deps = envoy_cc_platform_dep("thread_impl_lib"),
)

envoy_cc_posix_library(
name = "thread_impl_lib",
srcs = ["posix/thread_impl.cc"],
hdrs = ["posix/thread_impl.h"],
strip_include_prefix = "posix",
deps = [
":assert_lib",
"//include/envoy/thread:thread_interface",
],
)

envoy_cc_win32_library(
name = "thread_impl_lib",
srcs = ["win32/thread_impl.cc"],
hdrs = ["win32/thread_impl.h"],
strip_include_prefix = "win32",
deps = [
":assert_lib",
":macros",
"//include/envoy/thread:thread_interface",
],
)
Expand Down
36 changes: 36 additions & 0 deletions source/common/common/posix/thread_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#include "common/common/assert.h"
#include "common/common/thread_impl.h"

namespace Envoy {
namespace Thread {

ThreadImpl::ThreadImpl(std::function<void()> thread_routine) : thread_routine_(thread_routine) {
RELEASE_ASSERT(Logger::Registry::initialized(), "");
const int rc = pthread_create(&thread_handle_, nullptr,
[](void* arg) -> void* {
static_cast<ThreadImpl*>(arg)->thread_routine_();
return nullptr;
},
this);
RELEASE_ASSERT(rc == 0, "");
}

void ThreadImpl::join() {
const int rc = pthread_join(thread_handle_, nullptr);
RELEASE_ASSERT(rc == 0, "");
}

ThreadId currentThreadId() {
#ifdef __linux__
return syscall(SYS_gettid);
#elif defined(__APPLE__)
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return static_cast<int32_t>(tid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed; no need to add complexity here.

#else
#error "Enable and test pthread id retrieval code for you arch in pthread/thread_impl.cc"
#endif
}

} // namespace Thread
} // namespace Envoy
30 changes: 30 additions & 0 deletions source/common/common/posix/thread_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#pragma once

#include <functional>

#include "envoy/thread/thread.h"

namespace Envoy {
namespace Thread {

/**
* Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to
* unusable stack traces.
*/
class ThreadImpl : public Thread {
public:
ThreadImpl(std::function<void()> thread_routine);

/**
* Join on thread exit.
sesmith177 marked this conversation as resolved.
Show resolved Hide resolved
*/
void join() override;
ThreadHandle handle() const override { return thread_handle_; }

private:
std::function<void()> thread_routine_;
pthread_t thread_handle_;
};

} // namespace Thread
} // namespace Envoy
48 changes: 1 addition & 47 deletions source/common/common/thread.cc
Original file line number Diff line number Diff line change
@@ -1,61 +1,15 @@
#include "common/common/thread.h"

#ifdef __linux__
#include <sys/syscall.h>
#elif defined(__APPLE__)
#include <pthread.h>
#endif

#include <functional>

#include "common/common/assert.h"
#include "common/common/macros.h"
#include "common/common/thread_impl.h"

namespace Envoy {
namespace Thread {

/**
* Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to
* unusable stack traces.
*/
class ThreadImpl : public Thread {
public:
ThreadImpl(std::function<void()> thread_routine) : thread_routine_(thread_routine) {
RELEASE_ASSERT(Logger::Registry::initialized(), "");
int rc = pthread_create(&thread_id_, nullptr,
[](void* arg) -> void* {
static_cast<ThreadImpl*>(arg)->thread_routine_();
return nullptr;
},
this);
RELEASE_ASSERT(rc == 0, "");
}

void join() override {
int rc = pthread_join(thread_id_, nullptr);
RELEASE_ASSERT(rc == 0, "");
}

private:
std::function<void()> thread_routine_;
pthread_t thread_id_;
};

ThreadPtr ThreadFactoryImpl::createThread(std::function<void()> thread_routine) {
return std::make_unique<ThreadImpl>(thread_routine);
}

int32_t currentThreadId() {
#ifdef __linux__
return syscall(SYS_gettid);
#elif defined(__APPLE__)
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return static_cast<int32_t>(tid);
#else
#error "Enable and test pthread id retrieval code for you arch in thread.cc"
#endif
}

} // namespace Thread
} // namespace Envoy
2 changes: 0 additions & 2 deletions source/common/common/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
namespace Envoy {
namespace Thread {

typedef int32_t ThreadId;

/**
* Get current thread id.
*/
Expand Down
31 changes: 31 additions & 0 deletions source/common/common/win32/thread_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#include <process.h>

#include "common/common/assert.h"
#include "common/common/thread_impl.h"

namespace Envoy {
namespace Thread {

ThreadImpl::ThreadImpl(std::function<void()> thread_routine) : thread_routine_(thread_routine) {
Copy link
Contributor

@jmarantz jmarantz Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RELEASE_ASSERT(Logger::Registry::initialized(), "");
thread_handle_ =
reinterpret_cast<HANDLE>(::_beginthreadex(nullptr, 0,
[](void* arg) -> unsigned int {
static_cast<ThreadImpl*>(arg)->thread_routine_();
return 0;
},
this, 0, nullptr));
RELEASE_ASSERT(thread_handle_ != 0, "");
}

ThreadImpl::~ThreadImpl() { ::CloseHandle(thread_handle_); }

void ThreadImpl::join() {
const DWORD rc = ::WaitForSingleObject(thread_handle_, INFINITE);
RELEASE_ASSERT(rc == WAIT_OBJECT_0, "");
}

ThreadId currentThreadId() { return ::GetCurrentThreadId(); }

} // namespace Thread
} // namespace Envoy
31 changes: 31 additions & 0 deletions source/common/common/win32/thread_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#pragma once

#include <functional>

#include "envoy/thread/thread.h"

namespace Envoy {
namespace Thread {

/**
* Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to
* unusable stack traces.
*/
class ThreadImpl : public Thread {
public:
ThreadImpl(std::function<void()> thread_routine);
~ThreadImpl();

/**
* Join on thread exit.
*/
sesmith177 marked this conversation as resolved.
Show resolved Hide resolved
void join() override;
ThreadHandle handle() const override { return thread_handle_; }

private:
std::function<void()> thread_routine_;
HANDLE thread_handle_;
};

} // namespace Thread
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/grpc/google_async_client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "envoy/tracing/http_tracer.h"

#include "common/common/linked_object.h"
#include "common/common/thread.h"
#include "common/common/thread_annotations.h"
#include "common/tracing/http_tracer_impl.h"

Expand Down
2 changes: 1 addition & 1 deletion source/server/guarddog_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void GuardDogImpl::threadRoutine() {
} while (waitOrDetectStop());
}

WatchDogSharedPtr GuardDogImpl::createWatchDog(int32_t thread_id) {
WatchDogSharedPtr GuardDogImpl::createWatchDog(Thread::ThreadId thread_id) {
// Timer started by WatchDog will try to fire at 1/2 of the interval of the
// minimum timeout specified. loop_interval_ is const so all shared state
// accessed out of the locked section below is const (time_system_ has no
Expand Down
4 changes: 2 additions & 2 deletions source/server/guarddog_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
#include "envoy/server/watchdog.h"
#include "envoy/stats/scope.h"
#include "envoy/stats/stats.h"
#include "envoy/thread/thread.h"

#include "common/common/lock_guard.h"
#include "common/common/logger.h"
#include "common/common/thread.h"
#include "common/event/libevent.h"

#include "absl/types/optional.h"
Expand Down Expand Up @@ -54,7 +54,7 @@ class GuardDogImpl : public GuardDog {
}

// Server::GuardDog
WatchDogSharedPtr createWatchDog(int32_t thread_id) override;
WatchDogSharedPtr createWatchDog(Thread::ThreadId thread_id) override;
void stopWatching(WatchDogSharedPtr wd) override;

private:
Expand Down
Loading