-
-
Notifications
You must be signed in to change notification settings - Fork 369
lib: Make random number generation thread safe #6480
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,12 @@ | |
| * | ||
| * \brief GIS Library - Pseudo-random number generation | ||
| * | ||
| * (C) 2014 by the GRASS Development Team | ||
| * (C) 2014-2025 by the GRASS Development Team | ||
| * | ||
| * This program is free software under the GNU General Public License | ||
| * (>=v2). Read the file COPYING that comes with GRASS for details. | ||
| * | ||
| * \author Glynn Clements | ||
| * \authors Glynn Clements, Maris Nartiss (thread safety) | ||
| */ | ||
|
|
||
| #include <stdio.h> | ||
|
|
@@ -19,6 +19,10 @@ | |
| #include <grass/gis.h> | ||
| #include <grass/glocale.h> | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| #include <pthread.h> | ||
| #endif | ||
|
|
||
| #ifdef HAVE_GETTIMEOFDAY | ||
| #include <sys/time.h> | ||
| #else | ||
|
|
@@ -41,22 +45,38 @@ static const uint32 b0 = 0xB; | |
|
|
||
| static int seeded; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| static pthread_mutex_t lrand48_mutex = PTHREAD_MUTEX_INITIALIZER; | ||
| #endif | ||
|
|
||
| #define LO(x) ((x) & 0xFFFFU) | ||
| #define HI(x) ((x) >> 16) | ||
|
|
||
| /*! | ||
| * \brief Seed the pseudo-random number generator | ||
| * | ||
| * \param[in] seedval 32-bit integer used to seed the PRNG | ||
| * | ||
| * In a multi-threaded program, call `G_srand48()` once from the main | ||
| * thread *before* starting the worker threads. Subsequent calls will | ||
| * reset global seed state used by all threads. | ||
| */ | ||
| void G_srand48(long seedval) | ||
| { | ||
| uint32 x = (uint32) * (unsigned long *)&seedval; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_lock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| x2 = (uint16)HI(x); | ||
| x1 = (uint16)LO(x); | ||
| x0 = (uint16)0x330E; | ||
| seeded = 1; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_unlock(&lrand48_mutex); | ||
| #endif | ||
| } | ||
|
Comment on lines
64
to
80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling this from multiple threads will reset the global state. So, while the locking will prevent creation of a broken global state and it will prevent from reading a half-baked state, it will not work as expected. If you call this from multiple threads, they will replace the previous thread random state. Only the main thread should call this function and typically would call it once unless resetting the seed - that should be documented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct. I added a note to the documentation. Mutex guards should remain in place, but seed initialization always should be called only from main process/single thread. I called multiple times, it would result in "last wins" state. |
||
|
|
||
| /*! | ||
|
|
@@ -65,6 +85,10 @@ void G_srand48(long seedval) | |
| * A weak hash of the current time and PID is generated and used to | ||
| * seed the PRNG | ||
| * | ||
| * In a multi-threaded program, call `G_srand48_auto()` once from the main | ||
| * thread *before* starting the worker threads. Subsequent calls will | ||
| * reset global seed state used by all threads. | ||
| * | ||
| * \return generated seed value passed to G_srand48() | ||
| */ | ||
| long G_srand48_auto(void) | ||
|
|
@@ -128,47 +152,80 @@ static void G__next(void) | |
| /*! | ||
| * \brief Generate an integer in the range [0, 2^31) | ||
| * | ||
| * This function is thread-safe. | ||
| * | ||
| * \return the generated value | ||
| */ | ||
| long G_lrand48(void) | ||
| { | ||
| uint32 r; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_lock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| G__next(); | ||
| r = ((uint32)x2 << 15) | ((uint32)x1 >> 1); | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_unlock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| return (long)r; | ||
| } | ||
|
|
||
| /*! | ||
| * \brief Generate an integer in the range [-2^31, 2^31) | ||
| * | ||
| * This function is thread-safe. | ||
| * | ||
| * \return the generated value | ||
| */ | ||
| long G_mrand48(void) | ||
| { | ||
| uint32 r; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_lock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| G__next(); | ||
| r = ((uint32)x2 << 16) | ((uint32)x1); | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_unlock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| return (long)(int32)r; | ||
| } | ||
|
|
||
| /*! | ||
| * \brief Generate a floating-point value in the range [0,1) | ||
| * | ||
| * This function is thread-safe. | ||
| * | ||
| * \return the generated value | ||
| */ | ||
| double G_drand48(void) | ||
| { | ||
| double r = 0.0; | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_lock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| G__next(); | ||
| r += x2; | ||
| r *= 0x10000; | ||
| r += x1; | ||
| r *= 0x10000; | ||
| r += x0; | ||
| r /= 281474976710656.0; /* 2^48 */ | ||
|
|
||
| #ifdef HAVE_PTHREAD | ||
| pthread_mutex_unlock(&lrand48_mutex); | ||
| #endif | ||
|
|
||
| return r; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| """Test of gis library lrand48 PRNG thread-safety | ||
| @author Maris Nartiss | ||
| @author Gemini | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need. Human author is, or at least should be, the owner of the code in ways which matter to us. Approach to copyright differs in between countries and I would not even venture into it. If there is a important provenance info, it should go to the commit message (and/or PR description). We will not be inviting Gemini to a code sprint or to join the development team, nor expecting more insights into a code it generated comparing to any other code. |
||
| @copyright 2025 by the GRASS Development Team | ||
| @license This program is free software under the GNU General Public License (>=v2). | ||
| Read the file COPYING that comes with GRASS | ||
| for details | ||
| """ | ||
|
|
||
| import ctypes | ||
| import threading | ||
|
|
||
| from grass.gunittest.case import TestCase | ||
| from grass.gunittest.main import test | ||
|
|
||
| from grass.lib.gis import G_srand48, G_lrand48 | ||
|
|
||
|
|
||
| class Lrand48ThreadSafetyTestCase(TestCase): | ||
| """Test case for lrand48 thread-safety and reproducibility.""" | ||
|
|
||
| def test_thread_safety_and_reproducibility(self): | ||
| """Verify that multi-threaded execution produces the same set of | ||
| random numbers as single-threaded execution.""" | ||
|
|
||
| seed = 1337 | ||
| num_values = 10000 | ||
| num_threads = 4 | ||
| values_per_thread = num_values // num_threads | ||
|
|
||
| self.assertEqual( | ||
| num_values % num_threads, | ||
| 0, | ||
| "Total number of values must be divisible by the number of threads.", | ||
| ) | ||
|
|
||
| # --- Define ctypes function signatures --- | ||
| G_srand48.argtypes = [ctypes.c_long] | ||
| G_srand48.restype = None | ||
|
|
||
| G_lrand48.argtypes = [] | ||
| G_lrand48.restype = ctypes.c_long | ||
|
|
||
|
Comment on lines
+40
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this ctypesgen job? Isn't that already done? |
||
| # --- 1. Single-threaded execution --- | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While often seen in generated code, I think our experience is that these ASCII comment graphic is hard to maintain and inherently inconsistent (thinking about old code we cleaned in GRASS). |
||
| list_single = [] | ||
| G_srand48(seed) | ||
| for _ in range(num_values): | ||
| list_single.append(G_lrand48()) | ||
|
|
||
| # --- 2. Multi-threaded execution --- | ||
| list_multi_raw = [] | ||
| lock = threading.Lock() | ||
|
|
||
| def worker(): | ||
| """Calls G_lrand48 and appends the result to a shared list.""" | ||
| local_results = [] | ||
| for _ in range(values_per_thread): | ||
| # G_lrand48() itself is protected by a C-level mutex | ||
| local_results.append(G_lrand48()) | ||
|
|
||
| # Use a Python-level lock to safely extend the shared list | ||
| with lock: | ||
| list_multi_raw.extend(local_results) | ||
|
|
||
| # Reset the seed to ensure the sequence starts from the beginning | ||
| G_srand48(seed) | ||
|
|
||
| threads = [] | ||
| for _ in range(num_threads): | ||
| thread = threading.Thread(target=worker) | ||
| threads.append(thread) | ||
| thread.start() | ||
|
|
||
| for thread in threads: | ||
| thread.join() | ||
|
|
||
| # --- 3. Verification --- | ||
| self.assertEqual( | ||
| len(list_single), | ||
| len(list_multi_raw), | ||
| "Single-threaded and multi-threaded runs produced a different number of values.", | ||
| ) | ||
|
|
||
| # Check for duplicates in the multi-threaded list. The presence of duplicates | ||
| # would indicate that the C-level mutex failed and multiple threads | ||
| # received the same random number. | ||
| self.assertEqual( | ||
| len(list_multi_raw), | ||
| len(set(list_multi_raw)), | ||
| "Duplicate values found in multi-threaded run, indicating a race condition.", | ||
| ) | ||
|
|
||
| # The sorted lists of numbers must be identical. | ||
| # This confirms that although threads ran in parallel, the C-level mutex | ||
| # correctly serialized access to the PRNG, yielding the exact same | ||
| # block of numbers. | ||
| self.assertListEqual( | ||
| sorted(list_single), | ||
| sorted(list_multi_raw), | ||
| "The set of generated numbers differs between single-threaded and multi-threaded runs.", | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| test() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this should be replaced by the addition of
check_target(Threads::Threads HAVE_PTHREAD)to this file:grass/cmake/modules/CheckDependentLibraries.cmake
Line 222 in 76ee406