test.pl: Use END blocks to cancel watchdog timers #23678
Conversation
khwilliamson
force-pushed
the
watchdog
branch
from
d3de099 to
58b91ad
Compare
t/test.pl
Outdated
| kill($sig, $pid_to_kill); | ||
| }); | ||
|
|
||
| $cancel_string =~ s/HERE/$::watchdog_thread_->kill('KILL')->detach()/; |
There was a problem hiding this comment.
Still gets replaced badly:
$ perl -Mthreads -E '$cancel_string = "END{ HERE }"; our $watchdog_thread_ = threads->create(sub {}); $watchdog_thread_->join; $cancel_string =~ s/HERE/$::watchdog_thread_->kill('KILL')->detach()/; say $cancel_string'
END{ threads=SCALAR(0x562c8311de88)->kill(KILL)->detach() }
You need to escape the $ in the s/// replacement string.
There was a problem hiding this comment.
its interesting that nothing fails.
In the thread documentation, I noticed that unsafe signals preclude killing threads with a signal. But this is the normal method for clearing a watchdog timer under threads. I doubt that people are compiling with PERL_OLD_SIGNALS these days, so I didn't check for that, but its easy enough to check for the environment variable that does the same thing at runtime.
In doing follow up work to a7f3f23 "Turn off watchdog when done in tests", I realized that my solution was suboptimal, and that the code already existed in test.pl to do things better. What that code does is to create an END block to cancel the watchdog upon program exit. I think that is a better solution than to force the addition of explicit calls to watchdog(0). This commit creates a standard way to specify the code that does the cancellation, and to use it, not only when the END block does, but also when the timer is explicitly cancelled. That standard method is to create a string that basically gets evaled. This entailed making any watchdog thread object into a global so that a reference to it could be stringified for the eval. This means the calls to watchdog(0) that occur at the end of the file that were added in a7f3f23 aren't necessary. The END block takes care of it. The reason to keep them would be if we want to add a porting test that every watchdog is cleared.
The previous commit made these unnecessary. This commit removes them. I also removed a couple that occurred just before done_testing() and plan() that occur at the end of the program.
khwilliamson
force-pushed
the
watchdog
branch
from
58b91ad to
97d0c3e
Compare
|
In looking at things more closely, I discovered the possibility of unsafe signals, so I added a check against that I also changed so with threads you can have multiple alarms going |
I'll admit I was confused by this, since KILL isn't catchable, but threads uses the safe signal delivery mechanism. But the watchdog(0) signal still doesn't trigger the KILL handler set in the watchdog thread and I don't know why (yet anyway). |
| kill($sig, $pid_to_kill); | ||
| }); | ||
|
|
||
| #my $index = scalar @watchd |
There was a problem hiding this comment.
comment here seems leftover
|
Is the intent here to support nested watchdog timers? I don't see much other reason to mess around with the evalled END blocks here. If you didn't want nested watchdogs the original code could have worked, with an |
|
The intent was to allow whatever was possible before, even if not currently used. But I now realize that the prior implementation effectively had a limit of a single watchdog at a time that was cancellable. And the And, I re-read my comments added a while ago that convinced me, every watchdog should be cancelled, even at the end of the file, because it tells future maintainers there is a watchdog, and allows them to add new tests outside the control of it, or inside the control of it. Otherwise we could get flapping smoke tests. |
|
This is now withdrwan in favor of #23752 |
2 participants
In doing follow up work to a7f3f23
"Turn off watchdog when done in tests", I realized that my solution was
suboptimal, and that the code already existed in test.pl to do things
better.
What that code does is to create an END block to cancel the watchdog
upon program exit. I think that is a better solution than to force
the addition of explicit calls to watchdog(0). This commit creates a
standard way to specify the code that does the cancellation, and to use
it, not only when the END block does, but also when the timer is
explicitly cancelled.
This means the calls to watchdog(0) that occur at the end of the file
that were added in a7f3f23 aren't
necessary. The END block takes care of it. The reason to keep them is
if we want to add a porting test that every watchdog is cleared.