Merge branch 'main' into glob_filter

Author: bbockelm

.github/workflows/test.yml

@@ -29,6 +29,9 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: recursive
+    - uses: actions/setup-go@v5
+      with:
+        go-version: '1.23.5'
     - name: install deps
       run: |
         sudo curl -L https://xrootd.web.cern.ch/repo/RPM-GPG-KEY.txt -o /etc/apt/trusted.gpg.d/xrootd.asc

CMakeLists.txt

@@ -7,7 +7,9 @@ option( VALGRIND "Run select unit tests under valgrind" OFF )
 option( ASAN "Build the plugin with the address sanitizer" OFF )
 
 set( CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake )
-set( CMAKE_BUILD_TYPE Debug )
+if( "${CMAKE_BUILD_TYPE}" STREQUAL "" )
+  set( CMAKE_BUILD_TYPE Debug )
+endif()
 
 find_package( XRootD REQUIRED COMPONENTS UTILS SERVER )
 find_package( CURL REQUIRED )
@@ -39,6 +41,8 @@ endif()
 # Our custom Filesystem module creates the std::filesystem library target for
 # any special dependencies needed for using std::filesystem.
 find_package( Filesystem REQUIRED )
+# Similar setup for libatomic; required only on 32-bit systems
+find_package( Atomic REQUIRED )
 
 if( NOT XROOTD_EXTERNAL_TINYXML2 )
   include(FetchContent)
@@ -78,7 +82,7 @@ add_definitions( -D_FILE_OFFSET_BITS=64 )
 add_library(XrdS3Obj OBJECT src/CurlUtil.cc src/S3File.cc src/S3Directory.cc src/S3AccessInfo.cc src/S3FileSystem.cc src/AWSv4-impl.cc src/S3Commands.cc src/HTTPCommands.cc src/TokenFile.cc src/stl_string_utils.cc src/shortfile.cc src/logging.cc)
 set_target_properties(XrdS3Obj PROPERTIES POSITION_INDEPENDENT_CODE ON)
 target_include_directories(XrdS3Obj PRIVATE ${XRootD_INCLUDE_DIRS})
-target_link_libraries(XrdS3Obj ${XRootD_UTILS_LIBRARIES} ${XRootD_SERVER_LIBRARIES} CURL::libcurl OpenSSL::Crypto tinyxml2::tinyxml2 Threads::Threads std::filesystem)
+target_link_libraries( XrdS3Obj ${XRootD_UTILS_LIBRARIES} ${XRootD_SERVER_LIBRARIES} CURL::libcurl OpenSSL::Crypto tinyxml2::tinyxml2 Threads::Threads std::filesystem std::atomic )
 
 add_library(XrdS3 MODULE "$<TARGET_OBJECTS:XrdS3Obj>")
 target_link_libraries(XrdS3 XrdS3Obj)
@@ -116,11 +120,11 @@ else()
   set_target_properties( XrdOssFilter PROPERTIES OUTPUT_NAME "XrdOssFilter-${XRootD_PLUGIN_VERSION}" SUFFIX ".so" LINK_FLAGS "-Wl,--version-script=${CMAKE_SOURCE_DIR}/configs/export-lib-symbols" )
 endif()
 
-SET(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE PATH "Install path for libraries")
+include(GNUInstallDirs)
 
 install(
   TARGETS XrdS3 XrdHTTPServer
-  LIBRARY DESTINATION ${LIB_INSTALL_DIR}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
 )
 
 if( BUILD_TESTING )
@@ -137,7 +141,18 @@ if( BUILD_TESTING )
   target_link_libraries( XrdOssFilterTesting XrdOssFilterObj )
   target_include_directories( XrdOssFilterTesting INTERFACE ${XRootD_INCLUDE_DIRS} )
 
-  # Ensure that GTest is available
+  find_program(GoWrk go-wrk HINTS "$ENV{HOME}/go/bin")
+  if( NOT GoWrk )
+    # Try installing the go-wrk variable to generate a reasonable stress test
+    execute_process( COMMAND go install github.com/bbockelm/go-wrk@92dbe19
+      RESULT_VARIABLE go_install_result )
+    if( go_install_result EQUAL 0 )
+      find_program(GoWrk go-wrk HINTS "$ENV{HOME}/go/bin")
+    else()
+      message(ERROR "Failed to install the go-wrk binary" )
+    endif()
+  endif()
+
   if( NOT XROOTD_PLUGINS_EXTERNAL_GTEST )
     include( FetchContent )
     set( GTEST_URL "${CMAKE_CURRENT_SOURCE_DIR}/googletest-1.15.2.tar.gz" )

cmake/FindAtomic.cmake

@@ -0,0 +1,57 @@
+
+# Ideas come from
+#
+#  https://gitlab.kitware.com/cmake/cmake/-/issues/17834
+#
+# But applied to the use of libatomic instead of libstdc++fs.
+# The need to do this was highlighted as part of:
+#   https://github.com/PelicanPlatform/xrootd-s3-http/pull/81
+# The original driving use case was 32-bit builds; if we
+# decide to drop those, we can rip out the target
+
+include(CheckSourceCompiles)
+
+function( check_working_cxx_atomics varname )
+  CHECK_SOURCE_COMPILES( CXX "
+    #include <cstdlib>
+    #include <atomic>
+    #include <cstdint>
+
+    int main() {
+      std::atomic<uint8_t> a1;
+      std::atomic<uint16_t> a2;
+      std::atomic<uint32_t> a3;
+      std::atomic<uint64_t> a4;
+      return a1++ + a2++ + a3++ + a4++;
+    }" ${varname}
+  )
+endfunction( check_working_cxx_atomics varname )
+
+
+check_working_cxx_atomics( CXX_ATOMIC_NO_LINK_NEEDED )
+
+set( _found FALSE )
+if( CXX_ATOMIC_NO_LINK_NEEDED )
+  set( _found TRUE )
+else()
+  check_library_exists(atomic __atomic_fetch_add_4 "" HAVE_LIBATOMIC)
+  if (HAVE_LIBATOMIC)
+    set( OLD_CMAKE_REQUIRED_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES} )
+    list( APPEND CMAKE_REQUIRED_LIBRARIES "atomic" )
+    check_working_cxx_atomics( HAVE_CXX_ATOMICS_WITH_LIB )
+    set( CMAKE_REQUIRED_LIBRARIES ${OLD_CMAKE_REQUIRED_LIBRARIES} )
+    set( HAVE_CXX_ATOMICS_WITH_LIB TRUE )
+    set( _found TRUE )
+  endif()
+endif()
+
+add_library( std::atomic INTERFACE IMPORTED )
+
+if( HAVE_CXX_ATOMICS_WITH_LIB )
+  set_property( TARGET std::atomic APPEND PROPERTY INTERFACE_LINK_LIBRARIES atomic )
+endif()
+
+set( Atomic_FOUND ${_found} CACHE BOOL "TRUE if we can run a program using std::atomic" FORCE )
+if( Atomic_FIND_REQUIRED AND NOT Atomic_FOUND )
+    message( FATAL_ERROR "Cannot run simple program using std::atomic" )
+endif()

rpm/xrootd-s3-http.spec

@@ -1,28 +1,26 @@
-Name: xrootd-s3-http
-Version: 0.0.2
-Release: 1%{?dist}
-Summary: S3/HTTP filesystem plugins for xrootd
-
-Group: System Environment/Daemons
-License: BSD
-# Generated from:
-# git archive v%{version} --prefix=xrootd-s3-http-%{version}/ | gzip -7 > ~/rpmbuild/SOURCES/xrootd-s3-http-%{version}.tar.gz
-URL: https://github.com/pelicanplatform/xrootd-s3-http
-Source0: %{name}-%{version}.tar.gz
+Name:		xrootd-s3-http
+Version:        0.2.1
+Release:        1%{?dist}
+Summary:        S3/HTTP filesystem plugins for xrootd
+
+License:        Apache-2.0
+URL:            https://github.com/PelicanPlatform/%{name}
+Source0:        %{url}/archive/refs/tags/v%{version}/%{name}-%{version}.tar.gz
 
 %define xrootd_current_major 5
-%define xrootd_current_minor 5
+%define xrootd_current_minor 7
 %define xrootd_next_major 6
 
-BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
+BuildRequires: cmake3
+BuildRequires: gcc-c++
+BuildRequires: make
 BuildRequires: xrootd-server-libs >= 1:%{xrootd_current_major}
 BuildRequires: xrootd-server-libs <  1:%{xrootd_next_major}
 BuildRequires: xrootd-server-devel >= 1:%{xrootd_current_major}
 BuildRequires: xrootd-server-devel <  1:%{xrootd_next_major}
-BuildRequires: cmake3
-BuildRequires: gcc-c++
 BuildRequires: libcurl-devel
 BuildRequires: openssl-devel
+BuildRequires: tinyxml2-devel
 
 Requires: xrootd-server >= 1:%{xrootd_current_major}.%{xrootd_current_minor}
 Requires: xrootd-server <  1:%{xrootd_next_major}.0.0-1
@@ -34,19 +32,22 @@ Requires: xrootd-server <  1:%{xrootd_next_major}.0.0-1
 %setup -q
 
 %build
-%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo .
-make VERBOSE=1 %{?_smp_mflags}
+%cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DXROOTD_EXTERNAL_TINYXML2=ON
+cmake --build redhat-linux-build --verbose
 
 %install
-rm -rf $RPM_BUILD_ROOT
-make install DESTDIR=$RPM_BUILD_ROOT
+%cmake_install
 
 %files
-%defattr(-,root,root,-)
-%{_libdir}/libXrdS3*.so
-%{_libdir}/libXrdHTTPServer*.so
+%{_libdir}/libXrdHTTPServer-5.so
+%{_libdir}/libXrdS3-5.so
+%doc README.md
+%license LICENSE
 
 %changelog
+* Sat Feb 1 2025 Brian Bockelman <bbockelman@morgridge.org> - 0.2.1-1
+- Bump to upstream version 0.2.1.
+
 * Tue Nov 28 2023 Justin Hiemstra <jhiemstra@wisc.edu> - 0.0.2-1
 - Add HTTPServer plugin
 

src/CurlUtil.cc

@@ -34,7 +34,7 @@
 
 using namespace XrdHTTPServer;
 
-thread_local std::vector<CURL *> HandlerQueue::m_handles;
+thread_local std::stack<CURL *> HandlerQueue::m_handles;
 
 HandlerQueue::HandlerQueue() {
 	int filedes[2];
@@ -85,16 +85,16 @@ CURL *GetHandle(bool verbose) {
 }
 
 CURL *HandlerQueue::GetHandle() {
-	if (m_handles.size()) {
-		auto result = m_handles.back();
-		m_handles.pop_back();
+	if (!m_handles.empty()) {
+		CURL *result = m_handles.top();
+		m_handles.pop();
 		return result;
 	}
 
 	return ::GetHandle(false);
 }
 
-void HandlerQueue::RecycleHandle(CURL *curl) { m_handles.push_back(curl); }
+void HandlerQueue::RecycleHandle(CURL *curl) { m_handles.push(curl); }
 
 void HandlerQueue::Produce(HTTPRequest *handler) {
 	std::unique_lock<std::mutex> lk{m_mutex};
@@ -258,6 +258,7 @@ void CurlWorker::Run() {
 					   << curl_multi_strerror(mres);
 					m_logger.Log(LogMask::Debug, "Run", ss.str().c_str());
 				}
+				m_op_map.erase(curl);
 				op->Fail("E_CURL_LIB",
 						 "Unable to add operation to the curl multi-handle");
 				continue;

src/CurlUtil.hh

@@ -22,8 +22,8 @@
 #include <deque>
 #include <memory>
 #include <mutex>
+#include <stack>
 #include <unordered_map>
-#include <vector>
 
 // Forward dec'ls
 typedef void CURL;
@@ -58,7 +58,7 @@ class HandlerQueue {
 
   private:
 	std::deque<HTTPRequest *> m_ops;
-	thread_local static std::vector<CURL *> m_handles;
+	thread_local static std::stack<CURL *> m_handles;
 	std::condition_variable m_cv;
 	std::mutex m_mutex;
 	const static unsigned m_max_pending_ops{20};

src/S3Commands.cc

@@ -605,7 +605,7 @@ void AmazonS3Head::parseResponse() {
 	size_t current_newline = 0;
 	size_t next_newline = std::string::npos;
 	size_t last_character = headers.size();
-	while (current_newline != std::string::npos &&
+	while (headers.size() && current_newline != std::string::npos &&
 		   current_newline != last_character - 1) {
 		next_newline = headers.find("\r\n", current_newline + 2);
 		line = substring(headers, current_newline + 2, next_newline);

src/S3File.cc

@@ -547,6 +547,11 @@ S3File::DownloadBypass(off_t offset, size_t size, char *buffer) {
 	return std::make_tuple(-1, 0, true);
 }
 
+S3File::S3Cache::~S3Cache() {
+	std::unique_lock lk(m_mutex);
+	m_cv.wait(lk, [&] { return !m_a.m_inprogress && !m_b.m_inprogress; });
+}
+
 bool S3File::S3Cache::CouldUseAligned(off_t req, off_t cache) {
 	if (req < 0 || cache < 0) {
 		return false;
@@ -974,14 +979,24 @@ void S3File::S3Cache::Entry::Download(S3File &file) {
 	if (m_off + static_cast<off_t>(request_size) > file.content_length) {
 		request_size = file.content_length - m_off;
 	}
-	if (!m_request->SendRequest(m_off, m_cache_entry_size)) {
+	// This function is always called with m_mutex held; however,
+	// SendRequest can block if the threads are all busy; the threads
+	// will need to grab the lock to notify of completion.  So, we
+	// must release the lock here before calling a blocking function --
+	// otherwise deadlock may occur.
+	auto off = m_off;
+	m_parent.m_mutex.unlock();
+	if (!m_request->SendRequest(off, m_cache_entry_size)) {
+		m_parent.m_mutex.lock();
 		std::stringstream ss;
 		ss << "Failed to send GetObject command: "
 		   << m_request->getResponseCode() << "'"
 		   << m_request->getResultString() << "'";
 		file.m_log.Log(LogMask::Warning, "S3File::Read", ss.str().c_str());
 		m_failed = true;
 		m_request.reset();
+	} else {
+		m_parent.m_mutex.lock();
 	}
 }
 

src/S3File.hh

@@ -291,6 +291,10 @@ class S3File : public XrdOssDF {
 
 		// Trigger a blocking read from a given file
 		ssize_t Read(S3File &file, char *buffer, off_t offset, size_t size);
+
+		// Shutdown the cache; ensure all reads are completed before
+		// deleting the objects.
+		~S3Cache();
 	};
 	S3Cache m_cache;
 };

test/CMakeLists.txt

@@ -62,7 +62,7 @@ add_test(NAME HTTP::basic::setup
 set_tests_properties(HTTP::basic::setup
   PROPERTIES
     FIXTURES_SETUP HTTP::basic
-    ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR};SOURCE_DIR=${CMAKE_SOURCE_DIR}"
+    ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR};SOURCE_DIR=${CMAKE_SOURCE_DIR};XROOTD_BINDIR=${XRootD_DATA_DIR}/../bin"
 )
 
 add_test(NAME HTTP::basic::teardown
@@ -96,7 +96,7 @@ add_test(NAME S3::s3_basic::setup
 set_tests_properties(S3::s3_basic::setup
   PROPERTIES
     FIXTURES_SETUP S3::s3_basic
-    ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR};SOURCE_DIR=${CMAKE_SOURCE_DIR}"
+    ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR};SOURCE_DIR=${CMAKE_SOURCE_DIR};XROOTD_BINDIR=${XRootD_DATA_DIR}/../bin"
 )
 
 add_test(NAME S3::s3_basic::teardown
@@ -130,3 +130,17 @@ set_tests_properties(S3::s3_basic::stress_test
     ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR}"
     ATTACHED_FILES_ON_FAIL "${S3_BASIC_TEST_LOGS}"
 )
+
+#######################################
+# Stress-test using the go-wrk binary #
+#######################################
+
+add_test( NAME S3::s3_basic::gowrk_test
+  COMMAND "${CMAKE_CURRENT_SOURCE_DIR}/s3-gowrk-test.sh" s3_basic )
+
+set_tests_properties( S3::s3_basic::gowrk_test
+  PROPERTIES
+    FIXTURES_REQUIRED S3::s3_basic
+    ENVIRONMENT "BINARY_DIR=${CMAKE_BINARY_DIR};WRK_BIN=${GoWrk}"
+    ATTACHED_FILES_ON_FAIL "${S3_BASIC_TEST_LOGS}"
+)