Skip to content
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

Test dist/Storable/t/blessed.t fails with threaded perls < 5.14 under not too old Test2 #16395

Open
p5pRT opened this issue Jan 31, 2018 · 6 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 31, 2018

Migrated from rt.perl.org#132790 (status was 'open')

Searchable as RT132790$

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @andk

Three little birdies twittered the story to me, I lend them my voice
to let you know about it.

This is not about a bug in Storable. It's about a bug in old threaded
perls and how this bug unfolded over time.

The bug in perl itself was fixed in v5.19.2-116-g82b84d0.

Test​::More 1.001014 did not trigger the bug, it was only exposed
within dist/Storable/t/blessed.t through the changes in Test2.

But Test2 developed a workaround for the bug. Unfortunately that
workaround could only be applied to perls >= 5.14 not older ones. The
workaround is in EXODIST/Test2-0.000035-TRIAL.tar.gz. It can be
enforced by setting the environment variable T2_CHECK_DEPTH=1. So
older perls can run

  T2_CHECK_DEPTH=1 make test

in order to pass all Storable tests.

There is a contrived workaround for blessed.t itself. It avoids
triggering the bug by changing compilation order. I didn't understand
the birdies' explanation on the inner workings of this solution. Here
it is​:

% diff -U1 t/blessed.t{~,}

Inline Patch
--- t/blessed.t~        2018-01-30 20:44:59.935027614 +0000
+++ t/blessed.t 2018-01-30 20:51:01.133495863 +0000
@@ -18,4 +18,2 @@
 
-use Test::More;
-
 use Storable qw(freeze thaw store retrieve);
@@ -28,2 +26,4 @@
 
+use Test::More;
+
 {

If the patch would be applied, we could get rid of an inconvenience for everybody who taps into this test failure with old perls in the future\.

--
andreas

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @jkeenan

On Wed, 31 Jan 2018 04​:20​:26 GMT, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Three little birdies twittered the story to me, I lend them my voice
to let you know about it.

This is not about a bug in Storable. It's about a bug in old threaded
perls and how this bug unfolded over time.

The bug in perl itself was fixed in v5.19.2-116-g82b84d0.

Test​::More 1.001014 did not trigger the bug, it was only exposed
within dist/Storable/t/blessed.t through the changes in Test2.

But Test2 developed a workaround for the bug. Unfortunately that
workaround could only be applied to perls >= 5.14 not older ones. The
workaround is in EXODIST/Test2-0.000035-TRIAL.tar.gz. It can be
enforced by setting the environment variable T2_CHECK_DEPTH=1. So
older perls can run

T2_CHECK_DEPTH=1 make test

in order to pass all Storable tests.

There is a contrived workaround for blessed.t itself. It avoids
triggering the bug by changing compilation order. I didn't understand
the birdies' explanation on the inner workings of this solution. Here
it is​:

% diff -U1 t/blessed.t{,}
--- t/blessed.t
2018-01-30 20​:44​:59.935027614 +0000
+++ t/blessed.t 2018-01-30 20​:51​:01.133495863 +0000
@​@​ -18,4 +18,2 @​@​

-use Test​::More;
-
use Storable qw(freeze thaw store retrieve);
@​@​ -28,2 +26,4 @​@​

+use Test​::More;
+
{

If the patch would be applied, we could get rid of an inconvenience
for everybody who taps into this test failure with old perls in the
future.

I'm having trouble following you and, consequently, don't know how to reproduce the bug or test for its elimination once a correction has been made.

Are you saying that that if one combines a 7-year-old Perl with a contemporary Test-Simple distribution, you get a failure in a test for Storable in blead?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @andk

On Wed, 31 Jan 2018 05​:45​:16 -0800, "James E Keenan via RT" <perlbug-followup@​perl.org> said​:

  > On Wed, 31 Jan 2018 04​:20​:26 GMT, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Three little birdies twittered the story to me, I lend them my voice
to let you know about it.

This is not about a bug in Storable. It's about a bug in old threaded
perls and how this bug unfolded over time.

The bug in perl itself was fixed in v5.19.2-116-g82b84d0.

Test​::More 1.001014 did not trigger the bug, it was only exposed
within dist/Storable/t/blessed.t through the changes in Test2.

But Test2 developed a workaround for the bug. Unfortunately that
workaround could only be applied to perls >= 5.14 not older ones. The
workaround is in EXODIST/Test2-0.000035-TRIAL.tar.gz. It can be
enforced by setting the environment variable T2_CHECK_DEPTH=1. So
older perls can run

T2_CHECK_DEPTH=1 make test

in order to pass all Storable tests.

There is a contrived workaround for blessed.t itself. It avoids
triggering the bug by changing compilation order. I didn't understand
the birdies' explanation on the inner workings of this solution. Here
it is​:

% diff -U1 t/blessed.t{,}
--- t/blessed.t
2018-01-30 20​:44​:59.935027614 +0000
+++ t/blessed.t 2018-01-30 20​:51​:01.133495863 +0000
@​@​ -18,4 +18,2 @​@​

-use Test​::More;
-
use Storable qw(freeze thaw store retrieve);
@​@​ -28,2 +26,4 @​@​

+use Test​::More;
+
{

If the patch would be applied, we could get rid of an inconvenience
for everybody who taps into this test failure with old perls in the
future.

  > I'm having trouble following you and, consequently, don't know how to
  > reproduce the bug or test for its elimination once a correction has
  > been made.

Sorry, I'm not sure which information I left out. Maybe the fact that
Storable is upstream perl? Version 2.51 is on CPAN and should ever
somebody consider a new release, they would probably take the tests from
some recent perl. This is the reason why the report went to perlbug.

I'm pretty sure, no offense would be taken when you would not test the
outcome. From the point of view of bleadperl, you would probably prefer
just to ensure that the patch does not fail on bleadperl.

  > Are you saying that that if one combines a 7-year-old Perl with a
  > contemporary Test-Simple distribution, you get a failure in a test for
  > Storable in blead?

Yes.

perl 5.12.5 was released 2012-Nov-10, so is now 5 1/4 years old. Many
older perls would behave the same. On first sight we only see in the
matrix that old perls have more red color than newer perls. See
http​://matrix.cpantesters.org/?dist=Storable%202.51

Recipe once again​: take old perl, e.g. 5.12.5, compile it with threads,
combine it with any recent Test​::More, e.g. 1.302120 and test Storable
2.51 with it. That will fail on unpatched blessed.t and pass with the
patched one.

Adding an example​: take random pick from the matrix
http​://www.cpantesters.org/cpan/report/350affc0-6a71-11e6-856c-4bbe58b9f28c
this is a perl 5.8.5 and archname=x86_64-linux-thread-multi and Test​::More
1.302052 and blessed.t failed.

  > Thank you very much.

Thank you, I hope this clarifies what this is about,
--
andreas

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @jkeenan

On Wed, 31 Jan 2018 17​:12​:57 GMT, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

On Wed, 31 Jan 2018 05​:45​:16 -0800, "James E Keenan via RT"
<perlbug-followup@​perl.org> said​:

On Wed, 31 Jan 2018 04​:20​:26 GMT,
andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Three little birdies twittered the story to me, I lend them my voice
to let you know about it.

This is not about a bug in Storable. It's about a bug in old
threaded
perls and how this bug unfolded over time.

The bug in perl itself was fixed in v5.19.2-116-g82b84d0.

Test​::More 1.001014 did not trigger the bug, it was only exposed
within dist/Storable/t/blessed.t through the changes in Test2.

But Test2 developed a workaround for the bug. Unfortunately that
workaround could only be applied to perls >= 5.14 not older ones.
The
workaround is in EXODIST/Test2-0.000035-TRIAL.tar.gz. It can be
enforced by setting the environment variable T2_CHECK_DEPTH=1. So
older perls can run

T2_CHECK_DEPTH=1 make test

in order to pass all Storable tests.

There is a contrived workaround for blessed.t itself. It avoids
triggering the bug by changing compilation order. I didn't
understand
the birdies' explanation on the inner workings of this solution.
Here
it is​:

% diff -U1 t/blessed.t{,}
--- t/blessed.t
2018-01-30 20​:44​:59.935027614 +0000
+++ t/blessed.t 2018-01-30 20​:51​:01.133495863 +0000
@​@​ -18,4 +18,2 @​@​

-use Test​::More;
-
use Storable qw(freeze thaw store retrieve);
@​@​ -28,2 +26,4 @​@​

+use Test​::More;
+
{

If the patch would be applied, we could get rid of an inconvenience
for everybody who taps into this test failure with old perls in the
future.

I'm having trouble following you and, consequently, don't know how to
reproduce the bug or test for its elimination once a correction has
been made.

Sorry, I'm not sure which information I left out. Maybe the fact that
Storable is upstream perl? Version 2.51 is on CPAN and should ever
somebody consider a new release, they would probably take the tests
from
some recent perl. This is the reason why the report went to perlbug.

I'm pretty sure, no offense would be taken when you would not test the
outcome. From the point of view of bleadperl, you would probably
prefer
just to ensure that the patch does not fail on bleadperl.

Are you saying that that if one combines a 7-year-old Perl with a
contemporary Test-Simple distribution, you get a failure in a test
for
Storable in blead?

Yes.

perl 5.12.5 was released 2012-Nov-10, so is now 5 1/4 years old. Many
older perls would behave the same. On first sight we only see in the
matrix that old perls have more red color than newer perls. See
http​://matrix.cpantesters.org/?dist=Storable%202.51

Recipe once again​: take old perl, e.g. 5.12.5, compile it with
threads,
combine it with any recent Test​::More, e.g. 1.302120 and test Storable
2.51 with it. That will fail on unpatched blessed.t and pass with the
patched one.

Adding an example​: take random pick from the matrix
http​://www.cpantesters.org/cpan/report/350affc0-6a71-11e6-856c-
4bbe58b9f28c
this is a perl 5.8.5 and archname=x86_64-linux-thread-multi and
Test​::More
1.302052 and blessed.t failed.

Thank you very much.

Thank you, I hope this clarifies what this is about,

Please smoke branch smoke-me/jkeenan/132790-storable-blessed-t.

Since, as noted in the commit message, I don't know why this fix "works", we'll need further discussion and a better commit message before merging to blead.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @jkeenan

0001-TODO-Add-explanation-as-to-why-this-patch-works.patch
From 17e66fadd7c4d573e1b4cf6e1df5a4e5a4fc60c7 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 31 Jan 2018 13:25:22 -0500
Subject: [PATCH] TODO: Add explanation as to why this patch "works".

In RT #132790, Andreas reported that 3 unit tests in t/blessed.t were failing
under these conditions:

* pre-perl-5.14 perl
* modern Test-Simple distribution
* modern Storable from CPAN (which is behind blead)

For some reason TK moving the import of Test::More below %::weird_refs clears up
the test failures.  This appears to have no impact on blead.

The benefit of this will, of course, only become evident when a new version of
Storable is released to CPAN.
---
 dist/Storable/t/blessed.t | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dist/Storable/t/blessed.t b/dist/Storable/t/blessed.t
index 352eb5f..667746a 100644
--- a/dist/Storable/t/blessed.t
+++ b/dist/Storable/t/blessed.t
@@ -26,25 +26,25 @@ sub BEGIN {
     if ($ENV{PERL_CORE} and $Config{'extensions'} !~ /\bStorable\b/) {
         print "1..0 # Skip: Storable was not built\n";
         exit 0;
     }
 }
 
-use Test::More;
-
 use Storable qw(freeze thaw store retrieve);
 
 {
     %::weird_refs = (
         REF     => \(my $aref    = []),
         VSTRING => \(my $vstring = v1.2.3),
        'long VSTRING' => \(my $vstring = eval "v" . 0 x 300),
         LVALUE  => \(my $substr  = substr((my $str = "foo"), 0, 3)),
     );
 }
 
+use Test::More;
+
 my $test = 12;
 my $tests = $test + 23 + (2 * 6 * keys %::immortals) + (3 * keys %::weird_refs);
 plan(tests => $tests);
 
 package SHORT_NAME;
 
-- 
2.7.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants