Modify rcutils_getenv to be thread safe by ivanpauno · Pull Request #182 · ros2/rcutils

ivanpauno · 2019-09-17T21:09:34Z

Documentation about thread local storage:

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

wjwwood · 2019-09-17T21:25:56Z

include/rcutils/thread.h

+#ifndef _WIN32
+# define THREAD_LOCAL_STORAGE __thread
+#else
+# define THREAD_LOCAL_STORAGE __declspec(thread)


rcutils/include/rcutils/macros.h

Line 33 in e52f2cf

#define RCUTILS_THREAD_LOCAL __declspec(thread)

?

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.

Oh, I see we already had this

wjwwood · 2019-09-17T21:26:48Z

src/get_env.c

 #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];


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);

hidmic

Hmm, the fix itself is a fair solution, but I'm curious about rcutils_get_env. It wraps getenv_s when compiled for Windows and getenv for all the other platforms. But then, getenv_s ought to be available in all platforms as it is included in the C11 standard. Why not simply use getenv_s where appropriate?

wjwwood · 2019-09-17T21:39:58Z

getenv_s requires that you use your own storage, whereas getenv returns just a pointer to it.

Also, neither is thread-safe as far as I know:

This function is not required to be thread-safe. Another call to getenv, as well as a call to the POSIX functions setenv(), unsetenv(), and putenv() may invalidate the pointer returned by a previous call or modify the string obtained from a previous call.

https://en.cppreference.com/w/c/program/getenv

I'd recommend reading all of #30 if you haven't already.

hidmic · 2019-09-18T15:05:21Z

Alright, I see now. And I think the existing one is the best solution. In light of this, should #30 be closed as won't do?

wjwwood · 2019-09-23T22:18:51Z

In light of this, should #30 be closed as won't do?

I'm not sure. I think it's definitely not needed now, but maybe others have an opinion about it. We could close it or leave it open, doesn't matter to me. If we close it we can always reopen it.

clalancette · 2020-03-04T15:49:13Z

I think in light of what we discussed in ros2/rclcpp#987 (comment) , we should close this out in favor of a new PR. I'm going to go ahead and do that, but feel free to re-open if you disagree.

Do rcutils_getenv thread safe

03df75e

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

ivanpauno added enhancement New feature or request in review Waiting for review (Kanban column) labels Sep 17, 2019

ivanpauno requested review from hidmic and wjwwood September 17, 2019 21:09

ivanpauno self-assigned this Sep 17, 2019

wjwwood requested changes Sep 17, 2019

View reviewed changes

hidmic reviewed Sep 17, 2019

View reviewed changes

ivanpauno mentioned this pull request Sep 18, 2019

Delete rcl_impl_getenv, replaced by rcutils_get_env ros2/rcl#502

Merged

clalancette mentioned this pull request Mar 4, 2020

use rcutils_get_env instead of getenv() in node_options.cpp ros2/rclcpp#987

Closed

clalancette closed this Mar 4, 2020

ivanpauno deleted the ivanpauno/thread-safe-rcutils-get-env branch April 10, 2020 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify rcutils_getenv to be thread safe#182

Modify rcutils_getenv to be thread safe#182
ivanpauno wants to merge 1 commit intomasterfrom
ivanpauno/thread-safe-rcutils-get-env

ivanpauno commented Sep 17, 2019

Uh oh!

wjwwood Sep 17, 2019

Uh oh!

ivanpauno Sep 17, 2019

Uh oh!

ivanpauno Sep 17, 2019

Uh oh!

wjwwood Sep 17, 2019

Uh oh!

hidmic left a comment

Uh oh!

wjwwood commented Sep 17, 2019

Uh oh!

hidmic commented Sep 18, 2019

Uh oh!

wjwwood commented Sep 23, 2019

Uh oh!

clalancette commented Mar 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// 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);

Conversation

ivanpauno commented Sep 17, 2019

Uh oh!

wjwwood Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

ivanpauno Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

ivanpauno Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

wjwwood Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Sep 17, 2019

Uh oh!

hidmic commented Sep 18, 2019

Uh oh!

wjwwood commented Sep 23, 2019

Uh oh!

clalancette commented Mar 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants