bneradt commented on code in PR #13012: URL: https://github.com/apache/trafficserver/pull/13012#discussion_r2984535919
########## tests/gold_tests/logging/sigusr2.test.py: ########## @@ -130,9 +130,9 @@ def get_sigusr2_signal_command(self): # Configure the signaling of SIGUSR2 to traffic_server. tr1.Processes.Default.Command = diags_test.get_sigusr2_signal_command() tr1.Processes.Default.Return = 0 -tr1.Processes.Default.Ready = When.FileExists(diags_test.diags_log) - -# Configure process order. +# Configure process order: ts starts first, then rotate moves diags.log, +# then Default sends SIGUSR2. No Ready condition needed on Default since the +# StartBefore chain already ensures ts is fully started before rotate runs. tr1.Processes.Default.StartBefore(rotate_diags_log) rotate_diags_log.StartBefore(diags_test.ts) Review Comment: All three of these comments - the new source code comment, Copilot's review, and this latter one - are all a nonsensical dialog. They all miss the point to the Default Ready condition. Let's start with the process chain is the simple sequence of events it describes: * Start ATS via `rotate_diags_log.StartBefore(diags_test.ts)`. Nothing mysterious here. ATS is started and the diags.log **will** exist and be checked for "trafficserver is fully initialized" in it. That's a part of the ready condition for the ts process in the trafficserver.text.ext extension. * rotate_diags_log `mv`s the diag.log out of the way. This is simple: the diags.log will now be moved to some other filename, diags.log_old I believe. * Default then runs, sending a SIGUSR2. That's the chain of events. Now, the question for this patch is the Default Ready condition. The ready condition from before this patch was to ensure that the new diags.log is created. Thus, the command would send the SIGUSR2 to ATS as described above, and the Ready condition would wait for ATS to process the signal and recreate the diags.log file. Things should not proceed until that new diags.log file is created, thus the Ready. Make no mistake: we need to somehow await that new file being created to ensure proper timing alignment. I'm not entirely sure what problem is being solved here, but I assume that there is a race condition between the Default process sending the SIGUSR2, and the Ready awaiting the file creation? Maybe autest has a problem processing Ready when the process goes away, which it might do before the diags.log file is created? If that is the case, then let's do this: update the `ts_process_handler.py` script to take a path argument upon which to await the creation of the file before finishing. Maybe loop on the file creation with a .1 second sleep between iterations. Timeout after five seconds and exit with a non-zero if the file never gets created (no infinite loops, please). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
