Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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"
Dec 5, 2018
0b13893
Merge branch 'master' into add-windows-threads
Dec 5, 2018
428dd06
Pass ThreadIdPtr to createWatchdog
Dec 5, 2018
91c440f
cleanup
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()
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
5 changes: 2 additions & 3 deletions include/envoy/api/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,9 @@ class Api {
virtual std::string fileReadToEnd(const std::string& path) PURE;

/**
* Create a thread.
* @param thread_routine supplies the function to invoke in the thread.
* @return a reference to the ThreadFactory
*/
virtual Thread::ThreadPtr createThread(std::function<void()> thread_routine) PURE;
virtual Thread::ThreadFactory& threadFactory() PURE;
};

typedef std::unique_ptr<Api> ApiPtr;
Expand Down
8 changes: 4 additions & 4 deletions include/envoy/common/platform.h
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
#pragma once

// NOLINT(namespace-envoy)
#if !defined(_MSC_VER)
#define PACKED_STRUCT(definition, ...) definition, ##__VA_ARGS__ __attribute__((packed))

#else
#ifdef _MSC_VER
#include <malloc.h>

#define PACKED_STRUCT(definition, ...) \
__pragma(pack(push, 1)) definition, ##__VA_ARGS__; \
__pragma(pack(pop))

#else
#define PACKED_STRUCT(definition, ...) definition, ##__VA_ARGS__ __attribute__((packed))

#endif
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
4 changes: 2 additions & 2 deletions include/envoy/server/guarddog.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ class GuardDog {
* to avoid triggering the GuardDog. If no longer needed use the
* stopWatching() method to remove it from the list of watched objects.
*
* @param thread_id A numeric thread ID, like from Thread::currentThreadId()
* @param thread_id a Thread::ThreadIdPtr containing the system thread id
*/
virtual WatchDogSharedPtr createWatchDog(int32_t thread_id) PURE;
virtual WatchDogSharedPtr createWatchDog(Thread::ThreadIdPtr&& 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 const Thread::ThreadId& threadId() const PURE;
virtual MonotonicTime lastTouchTime() const PURE;
};

Expand Down
15 changes: 15 additions & 0 deletions include/envoy/thread/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@
namespace Envoy {
namespace Thread {

class ThreadId {
public:
virtual ~ThreadId() {}

virtual std::string debugString() const PURE;
virtual bool isCurrentThreadId() const PURE;
};

typedef std::unique_ptr<ThreadId> ThreadIdPtr;

class Thread {
public:
virtual ~Thread() {}
Expand All @@ -34,6 +44,11 @@ class ThreadFactory {
* @param thread_routine supplies the function to invoke in the thread.
*/
virtual ThreadPtr createThread(std::function<void()> thread_routine) PURE;

/**
* Return the current system thread ID
*/
virtual ThreadIdPtr currentThreadId() PURE;
};

/**
Expand Down
6 changes: 2 additions & 4 deletions source/common/api/api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 std::make_unique<Event::DispatcherImpl>(time_system, *this);
}

Filesystem::FileSharedPtr Impl::createFile(const std::string& path, Event::Dispatcher& dispatcher,
Expand All @@ -28,9 +28,7 @@ bool Impl::fileExists(const std::string& path) { return Filesystem::fileExists(p

std::string Impl::fileReadToEnd(const std::string& path) { return Filesystem::fileReadToEnd(path); }

Thread::ThreadPtr Impl::createThread(std::function<void()> thread_routine) {
return thread_factory_.createThread(thread_routine);
}
Thread::ThreadFactory& Impl::threadFactory() { return thread_factory_; }

} // namespace Api
} // namespace Envoy
2 changes: 1 addition & 1 deletion source/common/api/api_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Impl : public Api::Api {
Thread::BasicLockable& lock) override;
bool fileExists(const std::string& path) override;
std::string fileReadToEnd(const std::string& path) override;
Thread::ThreadPtr createThread(std::function<void()> thread_routine) override;
Thread::ThreadFactory& threadFactory() override;
Filesystem::Instance& fileSystem() { return file_system_; }

private:
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",
"envoy_select_boringssl",
Expand Down Expand Up @@ -180,12 +183,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
59 changes: 59 additions & 0 deletions source/common/common/posix/thread_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
#include "common/common/assert.h"
#include "common/common/thread_impl.h"

#if defined(__linux__)
#include <sys/syscall.h>
#endif

namespace Envoy {
namespace Thread {

namespace {

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

} // namespace

ThreadIdImplPosix::ThreadIdImplPosix(int64_t id) : id_(id) {}

std::string ThreadIdImplPosix::debugString() const { return std::to_string(id_); }

bool ThreadIdImplPosix::isCurrentThreadId() const { return id_ == getCurrentThreadId(); }

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, "");
}

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

ThreadIdPtr ThreadFactoryImplPosix::currentThreadId() {
return std::make_unique<ThreadIdImplPosix>(getCurrentThreadId());
}

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

#include <pthread.h>

#include <functional>

#include "envoy/thread/thread.h"

namespace Envoy {
namespace Thread {

class ThreadIdImplPosix : public ThreadId {
public:
ThreadIdImplPosix(int64_t id);

// Thread::ThreadId
std::string debugString() const override;
bool isCurrentThreadId() const override;

private:
int64_t id_;
};

/**
* 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.


// Thread::Thread
void join() override;

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

/**
* Implementation of ThreadFactory
*/
class ThreadFactoryImplPosix : public ThreadFactory {
public:
// Thread::ThreadFactory
ThreadPtr createThread(std::function<void()> thread_routine) override;
ThreadIdPtr currentThreadId() override;
};

} // namespace Thread
} // namespace Envoy
6 changes: 3 additions & 3 deletions source/common/common/stack_array.h
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#pragma once

#if !defined(WIN32)
#include <alloca.h>
#ifdef WIN32
#include <malloc.h>

#else
#include <malloc.h>
#include <alloca.h>
#endif

#include <stddef.h>
Expand Down
61 changes: 0 additions & 61 deletions source/common/common/thread.cc

This file was deleted.

Loading