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 4 commits
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
13 changes: 13 additions & 0 deletions include/envoy/thread/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,22 @@

#include "common/common/thread_annotations.h"

#if 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)
using ThreadId = int32_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

#else
using ThreadId = DWORD;
#endif

class Thread {
public:
virtual ~Thread() {}
Expand Down
24 changes: 22 additions & 2 deletions 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 @@ -179,12 +182,29 @@ envoy_cc_library(

envoy_cc_library(
name = "thread_lib",
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
43 changes: 43 additions & 0 deletions source/common/common/posix/thread_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#include "common/common/assert.h"
#include "common/common/thread_impl.h"

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

namespace Envoy {
namespace Thread {

ThreadImplPosix::ThreadImplPosix(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<ThreadImplPosix*>(arg)->thread_routine_();
return nullptr;
},
this);
RELEASE_ASSERT(rc == 0, "");
}

void ThreadImplPosix::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
41 changes: 41 additions & 0 deletions source/common/common/posix/thread_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#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 ThreadImplPosix : public Thread {
public:
ThreadImplPosix(std::function<void()> thread_routine);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jmarantz jmarantz Nov 30, 2018

Choose a reason for hiding this comment

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

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.


/**
* Join on thread exit.
sesmith177 marked this conversation as resolved.
Show resolved Hide resolved
*/
void join() override;

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

/**
* Implementation of ThreadFactory
*/
class ThreadFactoryImplPosix : public ThreadFactory {
public:
ThreadFactoryImplPosix() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

omit empty ctor.


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

} // namespace Thread
} // namespace Envoy
61 changes: 0 additions & 61 deletions source/common/common/thread.cc

This file was deleted.

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

typedef int32_t ThreadId;

/**
* Get current thread id.
*/
ThreadId currentThreadId();

/**
* Implementation of ThreadFactory
*/
class ThreadFactoryImpl : public ThreadFactory {
public:
ThreadFactoryImpl() {}

ThreadPtr createThread(std::function<void()> thread_routine) override;
};

/**
* Implementation of BasicLockable
*/
Expand Down
32 changes: 32 additions & 0 deletions source/common/common/win32/thread_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#include <process.h>

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

namespace Envoy {
namespace Thread {

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

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

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

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

} // namespace Thread
} // namespace Envoy
42 changes: 42 additions & 0 deletions source/common/common/win32/thread_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#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 ThreadImplWin32 : public Thread {
public:
ThreadImplWin32(std::function<void()> thread_routine);
~ThreadImplWin32();

/**
* Join on thread exit.
*/
sesmith177 marked this conversation as resolved.
Show resolved Hide resolved
void join() override;

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

/**
* Implementation of ThreadFactory
*/
class ThreadFactoryImplWin32 : public ThreadFactory {
public:
ThreadFactoryImplWin32() {}

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

} // 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
Loading