feat(cpp-client): Add entry points for SetEnv() and UnsetEnv() (#5987)

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

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"

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<std::string> 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

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"

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<std::string> 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

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