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

Modify rcutils_getenv to be thread safe #182

Closed
wants to merge 1 commit into from
Closed

Modify rcutils_getenv to be thread safe #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions include/rcutils/get_env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ extern "C"
*
* Environment variables will be truncated at 2048 characters on Windows.
*
* This function is not thread-safe.
*
* \param[in] env_name the name of the environment variable
* \param[out] env_value pointer to the value cstring, or "" if unset
* \return NULL on success (success can be returning an empty string)
Expand Down
33 changes: 33 additions & 0 deletions include/rcutils/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2019 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RCUTILS__THREAD_H_
#define RCUTILS__THREAD_H_

#ifdef __cplusplus
extern "C"
{
#endif

#ifndef _WIN32
# define THREAD_LOCAL_STORAGE __thread
#else
# define THREAD_LOCAL_STORAGE __declspec(thread)
Copy link
Member

Choose a reason for hiding this comment

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

rcutils/include/rcutils/macros.h

Line 33 in e52f2cf

#define RCUTILS_THREAD_LOCAL __declspec(thread)
?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the windows specific way of declaring a variable thread local. See https://docs.microsoft.com/en-us/cpp/cpp/thread?view=vs-2019.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see we already had this

#endif

#ifdef __cplusplus
}
#endif

#endif // RCUTILS__THREAD_H_
3 changes: 2 additions & 1 deletion src/get_env.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ extern "C"
#include <stdlib.h>

#include "rcutils/get_env.h"
#include "rcutils/thread.h"

#ifdef _WIN32
# define WINDOWS_ENV_BUFFER_SIZE 2048
static char __env_buffer[WINDOWS_ENV_BUFFER_SIZE];
THREAD_LOCAL_STORAGE static char __env_buffer[WINDOWS_ENV_BUFFER_SIZE];
Copy link
Member

Choose a reason for hiding this comment

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

This may cause memory allocations where before they did not occur, see:

rcutils/include/rcutils/error_handling.h

Lines 101 to 141 in e70143a

/// Forces initialization of thread-local storage if called in a newly created thread.
/**
* If this function is not called beforehand, then the first time the error
* state is set or the first time the error message is retrieved, the default
* allocator will be used to allocate thread-local storage.
*
* This function may or may not allocate memory.
* The system's thread-local storage implementation may need to allocate
* memory, since it usually has no way of knowing how much storage is needed
* without knowing how many threads will be created.
* Most implementations (e.g. C11, C++11, and pthread) do not have ways to
* specify how this memory is allocated, but if the implementation allows, the
* given allocator to this function will be used, but is otherwise unused.
* This only occurs when creating and destroying threads, which can be avoided
* in the "steady" state by reusing pools of threads.
*
* It is worth considering that repeated thread creation and destruction will
* result in repeated memory allocations and could result in memory
* fragmentation.
* This is typically avoided anyways by using pools of threads.
*
* In case an error is indicated by the return code, no error message will have
* been set.
*
* If called more than once in a thread, or after implicitly initialized by
* setting the error state, it will still return `RCUTILS_RET_OK`, even
* if the given allocator is invalid.
* Essentially this function does nothing if thread-local storage has already
* been called.
* If already initialized, the given allocator is ignored, even if it does not
* match the allocator used originally to initialize the thread-local storage.
*
* \return `RCUTILS_RET_OK` if successful, or
* \return `RCUTILS_RET_INVALID_ARGUMENT` if the allocator is invalid, or
* \return `RCUTILS_RET_BAD_ALLOC` if allocating memory fails, or
* \return `RCUTILS_RET_ERROR` if an unspecified error occurs.
*/
RCUTILS_PUBLIC
RCUTILS_WARN_UNUSED
rcutils_ret_t
rcutils_initialize_error_handling_thread_local_storage(rcutils_allocator_t allocator);

#endif // _WIN32


Expand Down