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.

Sign up for GitHub

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

Jump to bottom

Remove non portable use of pthread_t #563

Merged
merged 6 commits into from
Jan 3, 2020
Merged

Remove non portable use of pthread_t #563

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7dc9ac
wip: opaque pthread_t
nchong-at-aws Dec 17, 2019
8965512
Merge branch 'master' into opaque-pthread
nchong-at-aws Dec 19, 2019
baaa130
Address comments from @danielsn and @bretambrose
nchong-at-aws Dec 20, 2019
0e50a3b
Address comments from @graebm
nchong-at-aws Dec 30, 2019
7ea9dcb
Merge branch 'master' into opaque-pthread
bretambrose Jan 1, 2020
bd2c274
Merge branch 'master' into opaque-pthread
nchong-at-aws Jan 3, 2020
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
11 changes: 11 additions & 0 deletions include/aws/common/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/

#include <aws/common/common.h>
#include <aws/common/thread.h>

#define AWS_LOG_LEVEL_NONE 0
#define AWS_LOG_LEVEL_FATAL 1
Expand Down Expand Up @@ -237,6 +238,16 @@ void aws_logger_clean_up(struct aws_logger *logger);
AWS_COMMON_API
int aws_log_level_to_string(enum aws_log_level log_level, const char **level_string);

/**
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved
* Converts an aws_thread_id to a c-string. For portability, aws_thread_id
* must not be printed directly. Intended primarily to support building log
* lines that include the thread id in them. The return parameter `str` must
* point-to a char buffer of length `length == AWS_THREAD_ID_REPR_LEN`. The
* thread id representation is returned in `str`.
*/
AWS_COMMON_API
int aws_thread_id_to_string(aws_thread_id thread_id, char *str, size_t length);

/**
* Get subject name from log subject.
*/
Expand Down
25 changes: 19 additions & 6 deletions include/aws/common/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,27 @@ typedef union {
} aws_thread_once;
# define AWS_THREAD_ONCE_STATIC_INIT \
{ NULL }
typedef unsigned long aws_thread_id;
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved
#else
typedef pthread_once_t aws_thread_once;
# define AWS_THREAD_ONCE_STATIC_INIT PTHREAD_ONCE_INIT
typedef pthread_t aws_thread_id;
#endif

/*
* Number of chars to represent aws_thread_id as a string (2 hex chars per byte
* plus '\0' terminator). Needed for portable printing because pthread_t is
* opaque.
*/
#define AWS_THREAD_ID_REPR_LEN (sizeof(aws_thread_id) * 2 + 1)
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved

struct aws_thread {
struct aws_allocator *allocator;
enum aws_thread_detach_state detach_state;
#ifdef _WIN32
void *thread_handle;
unsigned long thread_id;
#else
pthread_t thread_id;
#endif
aws_thread_id thread_id;
};

AWS_EXTERN_C_BEGIN
Expand Down Expand Up @@ -86,7 +93,7 @@ int aws_thread_launch(
* Gets the id of thread
*/
AWS_COMMON_API
uint64_t aws_thread_get_id(struct aws_thread *thread);
aws_thread_id aws_thread_get_id(struct aws_thread *thread);

/**
* Gets the detach state of the thread. For example, is it safe to call join on
Expand All @@ -110,10 +117,16 @@ AWS_COMMON_API
void aws_thread_clean_up(struct aws_thread *thread);

/**
* returns the thread id of the calling thread.
* Returns the thread id of the calling thread.
*/
AWS_COMMON_API
aws_thread_id aws_thread_current_thread_id(void);
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved

/**
* Compare thread ids.
*/
AWS_COMMON_API
uint64_t aws_thread_current_thread_id(void);
bool aws_thread_thread_id_equal(aws_thread_id t1, aws_thread_id t2);

/**
* Sleeps the current thread by nanos.
Expand Down
20 changes: 16 additions & 4 deletions source/log_formatter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ static size_t s_advance_and_clamp_index(size_t current_index, int amount, size_t

return next_index;
}

/* Thread-local string representation of current thread id */
AWS_THREAD_LOCAL struct {
bool is_valid;
char repr[AWS_THREAD_ID_REPR_LEN];
Copy link
Contributor

Choose a reason for hiding this comment

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

If you also store the actual thread_id here, then this becomes the thing you take the address of in aws-c-io.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work, but would couple together c-io event loops and c common logging. Instead, how about changing the API for getting the current thread id to return a pointer to a thread-local aws_thread_id variable? The string repr can still be kept in logging, which is the only place it is needed.

// in posix/thread.c
AWS_THREAD_LOCAL struct {
    bool is_valid;
    aws_thread_id thread_id;
} tl_current_thread = {.is_valid = false};

const aws_thread_id *aws_thread_current_thread_id(void) {
    if (!tl_current_thread.is_valid) {
        tl_current_thread.thread_id = pthread_self();
        tl_current_thread.is_valid = true;
    }
    return &tl_current_thread.thread_id;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. "Many systems impose restrictions on the size of the thread-local memory block, in fact often rather tight limits." wikipedia. So it's not a great idea to make a bunch of thread-local storage variables, each of which caches one tiny thing.

  2. Is there a chance of the us trying to query the tl_current_thread after the thread is gone? I know the logger usually uses one logging thread. Other threads add their statements to a queue, and the logging thread drains the queue. If a thread logs something right before it exits, then the logging thread is going to try to get its name a little bit later and it will already be gone.

} tl_logging_thread_id = {.is_valid = false};

int aws_format_standard_log_line(struct aws_logging_standard_formatting_data *formatting_data, va_list args) {
size_t current_index = 0;

Expand Down Expand Up @@ -109,16 +116,21 @@ int aws_format_standard_log_line(struct aws_logging_standard_formatting_data *fo
/*
* Add thread id and user content separator (" - ")
*/
uint64_t current_thread_id = aws_thread_current_thread_id();
if (!tl_logging_thread_id.is_valid) {
aws_thread_id current_thread_id = aws_thread_current_thread_id();
if (aws_thread_id_to_string(current_thread_id, tl_logging_thread_id.repr, AWS_THREAD_ID_REPR_LEN)) {
return AWS_OP_ERR;
}
tl_logging_thread_id.is_valid = true;
}
int thread_id_written = snprintf(
formatting_data->log_line_buffer + current_index,
fake_total_length - current_index,
"] [%" PRIu64 "] ",
current_thread_id);
"] [%s] ",
tl_logging_thread_id.repr);
if (thread_id_written < 0) {
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
}

current_index = s_advance_and_clamp_index(current_index, thread_id_written, fake_total_length);
}

Expand Down
20 changes: 20 additions & 0 deletions source/logging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,26 @@ int aws_log_level_to_string(enum aws_log_level log_level, const char **level_str
return AWS_OP_SUCCESS;
}

int aws_thread_id_to_string(aws_thread_id thread_id, char *str, size_t length) {
AWS_ERROR_PRECONDITION(AWS_THREAD_ID_REPR_LEN == length);
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved
AWS_ERROR_PRECONDITION(str && AWS_MEM_IS_WRITABLE(str, length));
size_t current_index = 0;
unsigned char *bytes = (unsigned char *)&thread_id;
for (size_t i = sizeof(aws_thread_id); i != 0; --i) {
unsigned char c = bytes[i - 1];
int written = snprintf(str + current_index, length - current_index, "%02x", c);
nchong-at-aws marked this conversation as resolved.
Show resolved Hide resolved
if (written < 0) {
return AWS_OP_ERR;
}
current_index += written;
if (length <= current_index) {
return AWS_OP_ERR;
}
}
str[length - 1] = '\0';
return AWS_OP_SUCCESS;
}

#ifndef AWS_MAX_LOG_SUBJECT_SLOTS
# define AWS_MAX_LOG_SUBJECT_SLOTS 16u
#endif
Expand Down
16 changes: 9 additions & 7 deletions source/posix/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ void aws_thread_call_once(aws_thread_once *flag, void (*call_once)(void *), void
}

int aws_thread_init(struct aws_thread *thread, struct aws_allocator *allocator) {
thread->allocator = allocator;
thread->thread_id = 0;
thread->detach_state = AWS_THREAD_NOT_CREATED;
*thread = (struct aws_thread){.allocator = allocator, .detach_state = AWS_THREAD_NOT_CREATED};

return AWS_OP_SUCCESS;
}
Expand Down Expand Up @@ -176,8 +174,8 @@ int aws_thread_launch(
return AWS_OP_SUCCESS;
}

uint64_t aws_thread_get_id(struct aws_thread *thread) {
return (uintptr_t)thread->thread_id;
aws_thread_id aws_thread_get_id(struct aws_thread *thread) {
return thread->thread_id;
}

enum aws_thread_detach_state aws_thread_get_detach_state(struct aws_thread *thread) {
Expand Down Expand Up @@ -206,8 +204,12 @@ int aws_thread_join(struct aws_thread *thread) {
return AWS_OP_SUCCESS;
}

uint64_t aws_thread_current_thread_id(void) {
return (uintptr_t)pthread_self();
aws_thread_id aws_thread_current_thread_id(void) {
return pthread_self();
}

bool aws_thread_thread_id_equal(aws_thread_id t1, aws_thread_id t2) {
return pthread_equal(t1, t2) != 0;
}

void aws_thread_current_sleep(uint64_t nanos) {
Expand Down
10 changes: 7 additions & 3 deletions source/windows/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ int aws_thread_launch(
return AWS_OP_SUCCESS;
}

uint64_t aws_thread_get_id(struct aws_thread *thread) {
aws_thread_id aws_thread_get_id(struct aws_thread *thread) {
return thread->thread_id;
}

Expand All @@ -147,8 +147,12 @@ void aws_thread_clean_up(struct aws_thread *thread) {
thread->thread_handle = 0;
}

uint64_t aws_thread_current_thread_id(void) {
return (uint64_t)GetCurrentThreadId();
aws_thread_id aws_thread_current_thread_id(void) {
return GetCurrentThreadId();
}

bool aws_thread_thread_id_equal(aws_thread_id t1, aws_thread_id t2) {
return t1 == t2;
}

void aws_thread_current_sleep(uint64_t nanos) {
Expand Down
8 changes: 4 additions & 4 deletions tests/error_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,20 +359,20 @@ static int s_unknown_error_code_range_too_large_test_fn(struct aws_allocator *al
struct error_thread_test_data {
int thread_1_code;
int thread_1_get_last_code;
uint64_t thread_1_id;
aws_thread_id thread_1_id;
int thread_1_encountered_count;
int thread_2_code;
int thread_2_get_last_code;
int thread_2_encountered_count;
uint64_t thread_2_id;
aws_thread_id thread_2_id;
};

static void s_error_thread_test_thread_local_cb(int err, void *ctx) {
struct error_thread_test_data *cb_data = (struct error_thread_test_data *)ctx;

uint64_t thread_id = aws_thread_current_thread_id();
aws_thread_id thread_id = aws_thread_current_thread_id();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used again? If not, maybe move it inside the test?


if (thread_id == cb_data->thread_1_id) {
if (aws_thread_thread_id_equal(thread_id, cb_data->thread_1_id)) {
cb_data->thread_1_code = err;
cb_data->thread_1_get_last_code = aws_last_error();
cb_data->thread_1_encountered_count += 1;
Expand Down
20 changes: 13 additions & 7 deletions tests/logging/log_formatter_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,20 @@ int do_default_log_formatter_test(

char *thread_id_end = strstr(thread_id_start, "]");
ASSERT_TRUE(thread_id_end != NULL, "Could not find end of thread id in output line \"%s\"", buffer);
char *thread_id_end_copy = thread_id_end;

uint64_t current_thread_id = aws_thread_current_thread_id();
uint64_t logged_id = strtoumax(thread_id_start, &thread_id_end_copy, 10);
ASSERT_TRUE(
logged_id == current_thread_id,
"Expected logged thread id to be %" PRIu64 " but it was actually %" PRIu64 "",
current_thread_id,
ASSERT_TRUE((thread_id_end - thread_id_start + 1) == AWS_THREAD_ID_REPR_LEN, "Unexpected thread id length");
aws_thread_id current_thread_id = aws_thread_current_thread_id();
char repr[AWS_THREAD_ID_REPR_LEN];
ASSERT_SUCCESS(
aws_thread_id_to_string(current_thread_id, repr, AWS_THREAD_ID_REPR_LEN),
"Could not convert aws_thread_id to string repr");
char logged_id[AWS_THREAD_ID_REPR_LEN];
memcpy(logged_id, thread_id_start, AWS_THREAD_ID_REPR_LEN - 1);
logged_id[AWS_THREAD_ID_REPR_LEN - 1] = '\0';
ASSERT_SUCCESS(
strncmp(repr, logged_id, AWS_THREAD_ID_REPR_LEN),
"Expected logged thread id to be \"%s\" but it was actually \"%s\"",
repr,
logged_id);

/*
Expand Down
8 changes: 3 additions & 5 deletions tests/thread_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include <aws/testing/aws_test_harness.h>

struct thread_test_data {
uint64_t thread_id;
aws_thread_id thread_id;
};

static void s_thread_fn(void *arg) {
Expand All @@ -29,7 +29,6 @@ static void s_thread_fn(void *arg) {
static int s_test_thread_creation_join_fn(struct aws_allocator *allocator, void *ctx) {
(void)ctx;
struct thread_test_data test_data;
test_data.thread_id = 0;

struct aws_thread thread;
aws_thread_init(&thread, allocator);
Expand All @@ -38,9 +37,8 @@ static int s_test_thread_creation_join_fn(struct aws_allocator *allocator, void
ASSERT_INT_EQUALS(
AWS_THREAD_JOINABLE, aws_thread_get_detach_state(&thread), "thread state should have returned JOINABLE");
ASSERT_SUCCESS(aws_thread_join(&thread), "thread join failed");
ASSERT_INT_EQUALS(
test_data.thread_id,
aws_thread_get_id(&thread),
ASSERT_TRUE(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe have an assert false test here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To do that, we would need to manufacture an aws_thread_id value that we know is not the current thread. One way would be to create a new thread and then check against that.

aws_thread_thread_id_equal(test_data.thread_id, aws_thread_get_id(&thread)),
"get_thread_id should have returned the same id as the thread calling current_thread_id");
ASSERT_INT_EQUALS(
AWS_THREAD_JOIN_COMPLETED,
Expand Down