[Bug 34470] New: Real Time Holds Queue - make random numbers play nice with forked processes
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Bug ID: 34470 Summary: Real Time Holds Queue - make random numbers play nice with forked processes Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: emily.lamancusa@montgomerycountymd.gov QA Contact: testopia@bugs.koha-community.org In our system, we're noticing that the real-time holds queue is consistently sending a disproportionate number of holds to the same branches, although RandomizeHoldsQueueWeight is set to distribute holds randomly. The holds queue cron job often produced a "bubble" where one or two libraries would have a much longer holds queue than the others, but the "bubble" would move around between different branches from run to run. Since turning on the real-time holds queue, the "bubble" has stayed on the same two libraries for the full week that the real-time holds queue has been running. Other library systems have reported imbalanced distributions as well. Adding some logging revealed that the branches were being "randomized" to an identical order each time the holds queue is updated! This is consistent with the evidence that the same branches are always being assigned the most and second-most holds (respectively) in our production server. I was also able to replicate that same behavior on master, 22.11.x, and 22.05.x. Instructions to replicate are coming in the first comment, along with a patch to add the logging. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #1 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 154219 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154219&action=edit Bug 34470: [DO NOT PUSH] Add logging to watch randomization -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #2 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- To replicate: 1. Apply logging patch 2. Set system preferences: a. RealTimeHoldsQueue -> Enable b. RandomizeHoldsQueueWeight -> in random order 3. Watch the logs for the staff interface in ktd: ktd --shell koha-intra-err 4. Place a hold. Note that the logs display the branch list before and after it is randomized. 5. Place some more holds. Note that the branch order after randomization is identical each time. Bonus round 1: 6. Fill in some branches (I recommend at least 8 or so) for StaticHoldsQueueWeight 7. Place some more holds. 8. Note that the branch list before randomization is identical to the list in StaticHoldsQueueWeight, and the list after randomization is the same each time (but not necessarily identical to the randomized list that was produced when StaticHoldsQueueWeight was empty). Bonus round 2: 9. Add the following line to load_branches_to_pull_from in C4/HoldsQueue.pm: $logger->warn(rand()); 10. Place some more holds. 11. Note that rand() produces the same result each time, and the list after randomization is still the same each time (but not necessarily identical to the randomized list that was produced before the call to rand() was added). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #3 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- The logging makes it clear that the call to shuffle() in load_branches_to_pull_from is "randomizing" deterministically for some reason. According to the List::Utils docs, shuffle() depends on Perl's rand() function (unless an alternative method is set). I think the bug has to do with the scenario described here: https://stackoverflow.com/questions/58120618/how-to-get-random-number-in-for... We do fork at minimum one process to process a background job (or more depending on configuration after bug 32558). According to the above, if a parent process uses rand() and later spawns a child process which also calls rand(), every child process will inherit the same seed and thus produce an identical random number sequence unless the seed is reset with srand() after the fork. The official docs for rand() don't quite spell this out but they allude to the need to call srand() in a child process (though they also warn that srand() should not be called more than once per process). The results I'm seeing are consistent with this, and that would also explain why this doesn't happen in the cron job (which does not fork). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #4 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- The only thing I can't figure out is where (if?) randomization is happening in the parent process, which is supposed to be one of the conditions for the identical rand() results in the child process...but everything else fits. Either way, this is so deep in the weeds of Perl and parallel processing that I'd like to get more eyes on it (both for perspective and confirmation that others replicate the problem) before going further with it. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |normal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #5 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 154335 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154335&action=edit Bug 34470: Initialize random seed after spawning a child worker process When background_jobs_worker.pl spawns a new child process, it needs to explicitly reinitialize the random seed - otherwise each child process will inherit the same random seed from the parent process, and any randomization will produce identical results each time. This patch adds a call to srand immediately after the fork to reinitialize the seed. Note that child processes should not call srand with no parameter anywhere else, as the Perl documentation indicates that srand should not be called with no parameter more than once per process. To test: 1. Apply the logging patch only 2. Set system preferences: a. RealTimeHoldsQueue -> Enable b. RandomizeHoldsQueueWeight -> in random order 3. Watch the logs for the staff interface in ktd: ktd --shell koha-intra-err 4. Place a hold. Note that the logs display the branch list before and after it is randomized. 5. Place some more holds. Note that the branch order after randomization is identical each time. 6. Apply both patches and restart_all 7. Repeat steps 3-5. -> Note that the branch order before randomization hasn't changed -> Note that the branch order after randomization is now different each time. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154335|0 |1 is obsolete| | --- Comment #6 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 154336 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154336&action=edit Bug 34470: Initialize random seed after spawning a child worker process When background_jobs_worker.pl spawns a new child process, it needs to explicitly reinitialize the random seed - otherwise each child process will inherit the same random seed from the parent process, and any randomization will produce identical results each time. This patch adds a call to srand immediately after the fork to reinitialize the seed. Note that child processes should not call srand with no parameter anywhere else, as the Perl documentation indicates that srand should not be called with no parameter more than once per process. To test: 1. Apply the logging patch only 2. Set system preferences: a. RealTimeHoldsQueue -> Enable b. RandomizeHoldsQueueWeight -> in random order 3. Watch the logs for the staff interface in ktd: ktd --shell koha-intra-err 4. Place a hold. Note that the logs display the branch list before and after it is randomized. 5. Place some more holds. Note that the branch order after randomization is identical each time. 6. Apply both patches and restart_all 7. Repeat steps 3-5. -> Note that the branch order before randomization hasn't changed -> Note that the branch order after randomization is now different each time. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |emily.lamancusa@montgomeryc |ity.org |ountymd.gov -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154336|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 154339 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154339&action=edit Bug 34470: Initialize random seed after spawning a child worker process When background_jobs_worker.pl spawns a new child process, it needs to explicitly reinitialize the random seed - otherwise each child process will inherit the same random seed from the parent process, and any randomization will produce identical results each time. This patch adds a call to srand immediately after the fork to reinitialize the seed. Note that child processes should not call srand with no parameter anywhere else, as the Perl documentation indicates that srand should not be called with no parameter more than once per process. To test: 1. Apply the logging patch only 2. Set system preferences: a. RealTimeHoldsQueue -> Enable b. RandomizeHoldsQueueWeight -> in random order 3. Watch the logs for the staff interface in ktd: ktd --shell koha-intra-err 4. Place a hold. Note that the logs display the branch list before and after it is randomized. 5. Place some more holds. Note that the branch order after randomization is identical each time. 6. Apply both patches and restart_all 7. Repeat steps 3-5. -> Note that the branch order before randomization hasn't changed -> Note that the branch order after randomization is now different each time. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Signed Off |Passed QA --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Very small change, straightforward code, and excellently sleuthed! Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #10 from David Cook <dcook@prosentient.com.au> --- (In reply to Nick Clemens from comment #8)
Very small change, straightforward code, and excellently sleuthed!
+1 to the excellent sleuthing! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.03 released in| | Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m --- Comment #11 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.03 |23.11.00,23.05.03,22.11.09 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34470 --- Comment #12 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Nice work everyone! Pushed to 22.11.x for next release -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org