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
Can't run t/harness with a perl built for gprof #13040
Comments
From @nwc10Created by @nwc10I've built perl with -pg in the C flags to enabled profiling for gprof. ../cpan/IO-Compress/t/105oneshot-zip-store-only.t ................. ok The Perl code in question is this: unless (@ready) { my ( $h, $parser, $stash, @handles ) = @{ shift @ready }; With a little bit of annotation: Inline Patchdiff --git a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm b/cpan/Test-Harness
index 913aa92..4b7f173 100644
--- a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
+++ b/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
@@ -132,8 +132,15 @@ sub _iter {
}
unless (@ready) {
- return unless $sel->count;
+ my $count = $sel->count;
+ return unless $count;
@ready = $sel->can_read;
+ use Devel::Peek;
+ use Data::Dumper;
+ unless (defined $ready[0]) {
+ Dump $count;
+ print Dumper \@ready;
+ }
}
my ( $h, $parser, $stash, @handles ) = @{ shift @ready };
===( 48991;42 468/1750 292/1750 )=================================SV = IV(0x101ea70) at 0x101ea80 strace on that annotated version looks like this: read(3, "ok 1175\n", 65536) = 8 So whose bug is it? 1) pp_sselect, for not restarting on that signal? Nicholas Clark Perl Info
|
From @nwc10On Thu, Jun 20, 2013 at 05:55:05AM -0700, Nicholas Clark wrote:
I think that the answer is (3), and the correct fix is something like this: Inline Patchdiff --git a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm b/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
index 913aa92..a395a77 100644
--- a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
+++ b/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
@@ -133,7 +133,17 @@ sub _iter {
unless (@ready) {
return unless $sel->count;
- @ready = $sel->can_read;
+ # can_read can return an empty list if the select is
+ # interrupted by a signal. In which case restart the select
+ # immediately. There seems little point in trying to sleep
+ # (or similar) before retrying, as all those system calls
+ # would also get interrupted by signals.
+ my $retries;
+ while (!(@ready = $sel->can_read)) {
+ die "select for input was interrupted $retries times ($!). "
+ . "Something is badly wrong, so aborting"
+ if ++$retries > 1000;
+ }
}
my ( $h, $parser, $stash, @handles ) = @{ shift @ready };
Nicholas Clark |
From @LeontOn Thu, Jun 20, 2013 at 2:55 PM, Nicholas Clark
That would be the wrong default for all sorts of reasons, including
I don't see the contradiction. I do think it fails to indicate errors
Yeah, it needs to retry, but arguably only on EINTR. Leon |
The RT System itself - Status changed from 'new' to 'open' |
From @nwc10On Thu, Jun 20, 2013 at 05:16:07PM +0200, Leon Timmermans wrote:
On Thu, Jun 20, 2013 at 03:09:06PM +0100, Nicholas Clark wrote:
So this would be a better approach? Inline Patchdiff --git a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm b/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
index 913aa92..bdc21c0 100644
--- a/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
+++ b/cpan/Test-Harness/lib/TAP/Parser/Multiplexer.pm
@@ -133,7 +133,16 @@ sub _iter {
unless (@ready) {
return unless $sel->count;
- @ready = $sel->can_read;
+ # can_read can return an empty list if the select is
+ # interrupted by a signal. In which case restart the select
+ # immediately. There seems little point in trying to sleep
+ # (or similar) before retrying, as all those system calls
+ # would also get interrupted by signals.
+ my $retries;
+ while (!(@ready = $sel->can_read)) {
+ die "select for TAP input failed: $!"
+ unless $!{EINTR};
+ }
}
my ( $h, $parser, $stash, @handles ) = @{ shift @ready };
Nicholas Clark |
From @LeontOn Thu, Jun 20, 2013 at 5:49 PM, Nicholas Clark <nick@ccl4.org> wrote:
Yeah, that should work in this case (though as already pointed out on If we had to deal with timeouts too, you'd have to set $! to 0 before Leon |
From @ap* Nicholas Clark <nick@ccl4.org> [2013-06-20 17:50]:
Maybe like so? unless (@ready) { To my mind that breaks down the steps of the logic better. All the I’m not sure your comment regarding `sleep` is even right – it seems to -- |
From @nwc10On Fri, Jun 21, 2013 at 03:00:46AM +0200, Aristotle Pagaltzis wrote:
Well, apart from the fact that as-is there's no loop for `last` or `redo` :-) Presumably the inner { } should be a while (1) loop?
Yes. Others had pointed out tat the my $retries; should be removed. But I But I think that your layout is the thing that I was searching for, but
I was more thinking of things like retrying `fork`, or anything that would So yes, I guess my question is "is it obvious that it's not going to be a Nicholas Clark |
From @apHi Nick, * Nicholas Clark <nick@ccl4.org> [2013-06-21 11:20]:
hmm. I used the naked block on purpose here as I do not see this as a loop So I see this as straight-line logic in which `redo` is interspersed as Thus my choice to write it this particular way. But I concede that I may
Well, that makes sense for those kinds of things. :-) Your initial cut
My understanding is that “EINTR” should inherently suggest “tight loop”, But I can’t say how obvious this needs to be made for others. Regards, |
From @druud62On 21/06/2013 03:00, Aristotle Pagaltzis wrote:
Or like: unless (@ready) { -- |
From @LeontOn Fri, Jun 21, 2013 at 11:17 AM, Nicholas Clark <nick@ccl4.org> wrote:
In retrying fork it might make sense, but in this case I'd say it only Leon |
From @ap* Dr.Ruud <rvtol+usenet@isolution.nl> [2013-06-21 14:10]:
Yes, that’s the obvious first transformation of Nick’s original code. Regards, |
From @tamiasOn Fri, Jun 21, 2013 at 10:17:23AM +0100, Nicholas Clark wrote:
The loop control statements work with a bare block. Basic BLOCKs A BLOCK by itself (labeled or not) is semantically equivalent to a Ronald |
@Leont is this still a bug or was it sorted somewhere in the past. Either way it should be opened over here, right? https://rt.cpan.org/Dist/Display.html?Name=Test-Harness |
Yeah it's still a bug. Reporting here on github is fine, I should probably move that queue anyway. |
Migrated from rt.perl.org#118549 (status was 'open')
Searchable as RT118549$
The text was updated successfully, but these errors were encountered: