diff --git a/cpp-client/deephaven/dhcore/CMakeLists.txt b/cpp-client/deephaven/dhcore/CMakeLists.txt index 219316d3eb0..1a0626757a3 100644 --- a/cpp-client/deephaven/dhcore/CMakeLists.txt +++ b/cpp-client/deephaven/dhcore/CMakeLists.txt @@ -23,6 +23,7 @@ set(ALL_FILES src/immerutil/immer_column_source.cc src/interop/testapi/basic_interop_interactions.cc src/interop/interop_util.cc + src/interop/utility_interop.cc src/ticking/barrage_processor.cc src/ticking/immer_table_state.cc src/ticking/index_decoder.cc @@ -52,6 +53,7 @@ set(ALL_FILES include/public/deephaven/dhcore/container/row_sequence.h include/public/deephaven/dhcore/interop/testapi/basic_interop_interactions.h include/public/deephaven/dhcore/interop/interop_util.h + include/public/deephaven/dhcore/interop/utility_interop.h include/public/deephaven/dhcore/ticking/barrage_processor.h include/public/deephaven/dhcore/ticking/ticking.h include/public/deephaven/dhcore/utility/cython_support.h diff --git a/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/interop/utility_interop.h b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/interop/utility_interop.h new file mode 100644 index 00000000000..4885b8cdfa5 --- /dev/null +++ b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/interop/utility_interop.h @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending + */ +#pragma once + +#include "deephaven/dhcore/interop/interop_util.h" + +extern "C" { +void deephaven_dhcore_utility_GetEnv(const char *envname, + deephaven::dhcore::interop::StringHandle *string_handle, + deephaven::dhcore::interop::StringPoolHandle *string_pool_handle, + deephaven::dhcore::interop::ErrorStatus *status); + +void deephaven_dhcore_utility_SetEnv(const char *envname, const char *value, + deephaven::dhcore::interop::ErrorStatus *status); + +void deephaven_dhcore_utility_UnsetEnv(const char *envname, + deephaven::dhcore::interop::ErrorStatus *status); +} // extern "C" diff --git a/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/utility/utility.h b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/utility/utility.h index 40ad15f3a9b..574b6beeea0 100644 --- a/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/utility/utility.h +++ b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/utility/utility.h @@ -217,6 +217,19 @@ std::string Basename(std::string_view path); */ [[nodiscard]] std::optional GetEnv(const std::string& envname); +/** + * Sets a value in the environment. + * @param envname the key + * @param value the value to set in the environment + */ +void SetEnv(const std::string& envname, const std::string& value); + +/** + * Unsets a value in the environment. + * @param envname the key to unset + */ +void UnsetEnv(const std::string& envname); + /** * Enables or disables echo for stdin. * @param enable true to enable, false to disable diff --git a/cpp-client/deephaven/dhcore/src/interop/utility_interop.cc b/cpp-client/deephaven/dhcore/src/interop/utility_interop.cc new file mode 100644 index 00000000000..cb787dc1a5a --- /dev/null +++ b/cpp-client/deephaven/dhcore/src/interop/utility_interop.cc @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending + */ +#include "deephaven/dhcore/interop/utility_interop.h" + +#include "deephaven/dhcore/interop/interop_util.h" +#include "deephaven/dhcore/utility/utility.h" + +using deephaven::dhcore::interop::ErrorStatus; +using deephaven::dhcore::interop::StringHandle; +using deephaven::dhcore::interop::StringPoolBuilder; +using deephaven::dhcore::interop::StringPoolHandle; +using deephaven::dhcore::utility::GetEnv; +using deephaven::dhcore::utility::SetEnv; +using deephaven::dhcore::utility::UnsetEnv; + +extern "C" { +void deephaven_dhcore_utility_GetEnv(const char *envname, + StringHandle *string_handle, StringPoolHandle *string_pool_handle, ErrorStatus *status) { + status->Run([=]() { + StringPoolBuilder builder; + auto result = GetEnv(envname); + if (result.has_value()) { + *string_handle = builder.Add(*result); + } + // If envname is not found in the environment, caller will see an empty builder + *string_pool_handle = builder.Build(); + }); +} + +void deephaven_dhcore_utility_SetEnv(const char *envname, const char *value, ErrorStatus *status) { + status->Run([=]() { + SetEnv(envname, value); + }); +} + +void deephaven_dhcore_utility_UnsetEnv(const char *envname, ErrorStatus *status) { + status->Run([=]() { + UnsetEnv(envname); + }); +} +} // extern "C" diff --git a/cpp-client/deephaven/dhcore/src/utility/utility_platform_specific.cc b/cpp-client/deephaven/dhcore/src/utility/utility_platform_specific.cc index d75fc72e37a..f0b83a74dd7 100644 --- a/cpp-client/deephaven/dhcore/src/utility/utility_platform_specific.cc +++ b/cpp-client/deephaven/dhcore/src/utility/utility_platform_specific.cc @@ -104,21 +104,99 @@ std::string GetHostname() { #endif } +namespace { +/** + * This method wraps a function-local static mutex. The purpose of this mutex is to + * synchronize the calls GetEnv(), SetEnv(), and UnsetEnv(). + * + * The rationale for synchronizing these calls is that they are not guaranteed + * reentrant on either Linux or Windows. On Linux, "man getenv" says + * + * The implementation of getenv() is not required to be reentrant. The + * string pointed to by the return value of getenv() may be statically al‐ + * located, and can be modified by a subsequent call to getenv(), + * putenv(3), setenv(3), or unsetenv(3). + * + * On Windows, https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-wputenv?view=msvc-170 + * says + * + * The _putenv and _getenv families of functions are not thread-safe. + * _getenv could return a string pointer while _putenv is modifying the string, + * causing random failures. Make sure that calls to these functions are synchronized. + * + * Finally, we use a function-local static rather than a global so we don't have to think about / + * worry about whether global initialization was done correctly on the mutex object. + * This "worry" might be unfounded. + */ +std::mutex &MutexForEnvInvocations() { + static std::mutex the_mutex; + return the_mutex; +} +} // namespace + std::optional GetEnv(const std::string& envname) { #if defined(__unix__) + // Protect against concurrent XXXEnv() calls. See comment in MutexForEnvInvocations() + std::unique_lock guard(MutexForEnvInvocations()); const char* ret = getenv(envname.c_str()); if (ret != nullptr) { return std::string(ret); } return {}; #elif defined(_WIN32) + // Protect against concurrent XXXEnv() calls. See comment in MutexForEnvInvocations() + std::unique_lock guard(MutexForEnvInvocations()); static char ret[1024]; size_t len; const errno_t err = getenv_s(&len, ret, sizeof(ret), envname.c_str()); - if (err == 0) { - return std::string(ret); + // Return an unset optional if there's an error, or if the key is not found. + // len == 0 means "key not found" on Windows. + if (err != 0 || len == 0) { + return {}; } - return {}; + return std::string(ret); +#else +#error "Unsupported configuration" +#endif +} + +void SetEnv(const std::string& envname, const std::string& value) { +#if defined(__unix__) + // Protect against concurrent XXXEnv() calls. See comment in MutexForEnvInvocations() + std::unique_lock guard(MutexForEnvInvocations()); + + auto res = setenv(envname.c_str(), value.c_str(), 1); + if (res != 0) { + auto message = fmt::format("setenv failed, error={}", strerror(errno)); + throw std::runtime_error(DEEPHAVEN_LOCATION_STR(message)); + } +#elif defined(_WIN32) + // Protect against concurrent XXXEnv() calls. See comment in MutexForEnvInvocations() + std::unique_lock guard(MutexForEnvInvocations()); + + auto res = _putenv_s(envname.c_str(), value.c_str()); + if (res != 0) { + int lasterr = WSAGetLastError(); + auto message = fmt::format("_putenv_s failed: error code {}", lasterr); + throw std::runtime_error(DEEPHAVEN_LOCATION_STR(message)); + } +#else +#error "Unsupported configuration" +#endif +} + +void UnsetEnv(const std::string& envname) { +#if defined(__unix__) + // Protect against concurrent XXXEnv() calls. See comment in MutexForEnvInvocations() + std::unique_lock guard(MutexForEnvInvocations()); + + auto res = unsetenv(envname.c_str()); + if (res != 0) { + auto message = fmt::format("unsetenv failed, error={}", strerror(errno)); + throw std::runtime_error(DEEPHAVEN_LOCATION_STR(message)); + } +#elif defined(_WIN32) + SetEnv(envname, ""); #else #error "Unsupported configuration" #endif diff --git a/cpp-client/deephaven/tests/src/utility_test.cc b/cpp-client/deephaven/tests/src/utility_test.cc index f42dfdc51d8..aa81b68afc6 100644 --- a/cpp-client/deephaven/tests/src/utility_test.cc +++ b/cpp-client/deephaven/tests/src/utility_test.cc @@ -11,6 +11,8 @@ using deephaven::dhcore::utility::GetEnv; using deephaven::dhcore::utility::GetTidAsString; using deephaven::dhcore::utility::GetHostname; using deephaven::dhcore::utility::ObjectId; +using deephaven::dhcore::utility::SetEnv; +using deephaven::dhcore::utility::UnsetEnv; namespace deephaven::client::tests { TEST_CASE("Base64encode", "[utility]") { @@ -66,12 +68,43 @@ TEST_CASE("GetHostname", "[utility]") { // This isn't much of a test, but if it can compile on all supported // platforms (Linux and Windows) then that is at least a sanity check // (that the entry point exists). For now we just visually spot-check -// that ireturns the right value. +// that it returns the right value. TEST_CASE("GetEnv", "[utility]") { - auto path = GetEnv("PATH"); - // Very suspect if neither Windows nor Linux has a PATH set in their - // environment. - REQUIRE(path.has_value()); - fmt::println("PATH is: {}", *path); +#if defined(__unix__) + const char *expected_key = "PATH"; +#elif defined(_WIN32) + const char *expected_key = "OS"; +#else +#error "Unsupported configuration" +#endif + + auto value = GetEnv(expected_key); + REQUIRE(value.has_value()); + fmt::println("{} is: {}", expected_key, *value); +} + +// Confirm that SetEnv leaves something that GetEnv can find. +TEST_CASE("SetEnv", "[utility]") { + std::string unlikely_key = "Deephaven__serious_realtime_data_tools"; + std::string unlikely_value = "query_engine_APIs_and_user_interfaces"; + { + auto value = GetEnv(unlikely_key); + if (value.has_value()) { + fmt::println(std::cerr, "unexpected value is {}", *value); + } + REQUIRE(!value.has_value()); + } + + SetEnv(unlikely_key, unlikely_value); + { + auto value = GetEnv(unlikely_key); + REQUIRE(unlikely_value == value); + } + + UnsetEnv(unlikely_key); + { + auto value = GetEnv(unlikely_key); + REQUIRE(!value.has_value()); + } } } // namespace deephaven::client::tests