Storable fails tests with Log::Agent installed

[p5pRT]

Migrated from rt.perl.org#133228 (status was 'resolved')

Searchable as RT133228$

[p5pRT]

From @haarg

Storable rethrows errors from its hooks\, STORABLE_freeze\, STORABLE_thaw\, and STORABLE_attach. If those errors are refs\, they should be preserved in their original form. Tests were added to ensure this in 06f586d. However\, when Log​::Agent is installed\, Storable uses it rather than just calling Carp​::croak. Log​::Agent's logcroak function always stringifies the errors it gets when it throws them. This causes test failures in many cases when the dual life dist is installed from CPAN.

A few CPAN Testers reports from this issue​: https://www.cpantesters.org/cpan/report/00761375-6c0c-1014-946e-8eaf8038f308 https://www.cpantesters.org/cpan/report/8e51c42a-4cf5-11e8-b010-a35ca3102b3d https://www.cpantesters.org/cpan/report/c77a5184-4d6f-11e8-9bc9-021ad4cd031b https://www.cpantesters.org/cpan/report/84c75978-5e44-11e8-bbaf-bdf0d2eb68fb

This is a bug in Log​::Agent\, but since it is long standing behavior in a non-core module\, I think it's reasonable to work around it in Storable. The optional use of Log​::Agent seems pretty questionable to me in general\, but since it has been there for as long as Storable has existed in core it seemed reasonable to leave it in place.

The simplest workaround I could think of was to trap the error thrown by Log​::Agent\, then throw the original arguments given to our logcroak.

Attached is a patch written against the sawyer/storable branch\, which is where the current CPAN releases have been cut from\, since blead is frozen.

---

---

Flags​:   category=library   severity=low   module=Storable

---

[p5pRT]

From @haarg

0001-fix-Storable-rethrowing-refs-when-Log-Agent-is-insta.patch

```diff From 33d66ce8854efa707e448016376937a60197e914 Mon Sep 17 00:00:00 2001 From: Graham Knop Date: Mon, 7 May 2018 13:50:53 +0200 Subject: [PATCH] fix Storable rethrowing refs when Log::Agent is installed Storable rethrows errors from its hooks, STORABLE_freeze, STORABLE_thaw, and STORABLE_attach. If those errors are refs, they should be preserved in their original form. Tests were added to ensure this in 06f586d. However, when Log::Agent is installed, Storable uses it rather than just calling Carp::croak. Log::Agent's logcroak function always stringifies the errors it gets when it throws them. This causes test failures in many cases when the dual life dist is installed from CPAN. This is a bug in Log::Agent, but since it is long standing behavior in a non-core module, it's reasonable to work around it in Storable. A simple workaround is to trap the error thrown by Log::Agent, and throw the original arguments given to our logcroak. --- dist/Storable/__Storable__.pm | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/dist/Storable/__Storable__.pm b/dist/Storable/__Storable__.pm index 71c669daaf..b2ff8e1a53 100644 --- a/dist/Storable/__Storable__.pm +++ b/dist/Storable/__Storable__.pm @@ -39,6 +39,8 @@ package Storable; $recursion_limit_hash = 256 unless defined $recursion_limit_hash; +use Carp; + BEGIN { if (eval { local $SIG{__DIE__}; @@ -54,16 +56,23 @@ BEGIN # provide a fallback implementation. # unless ($Storable::{logcroak} && *{$Storable::{logcroak}}{CODE}) { - require Carp; + *logcroak = \&Carp::croak; + } + else { + # Log::Agent's logcroak always adds a newline to the error it is + # given. This breaks refs getting thrown. We can just discard what + # it throws (but keep whatever logging it does) and throw the original + # args. + no warnings 'redefine'; + my $logcroak = \&logcroak; *logcroak = sub { - Carp::croak(@_); + my @args = @_; + eval { &$logcroak }; + Carp::croak(@args); }; } unless ($Storable::{logcarp} && *{$Storable::{logcarp}}{CODE}) { - require Carp; - *logcarp = sub { - Carp::carp(@_); - }; + *logcarp = \&Carp::carp; } } -- 2.17.0 ```

[p5pRT]

From @tonycoz

On Mon\, 28 May 2018 06​:27​:54 -0700\, haarg wrote​:

Storable rethrows errors from its hooks\, STORABLE_freeze\, STORABLE_thaw\, and STORABLE_attach. If those errors are refs\, they should be preserved in their original form. Tests were added to ensure this in 06f586d. However\, when Log​::Agent is installed\, Storable uses it rather than just calling Carp​::croak. Log​::Agent's logcroak function always stringifies the errors it gets when it throws them. This causes test failures in many cases when the dual life dist is installed from CPAN.

A few CPAN Testers reports from this issue​: https://www.cpantesters.org/cpan/report/00761375-6c0c-1014-946e-8eaf8038f308 https://www.cpantesters.org/cpan/report/8e51c42a-4cf5-11e8-b010-a35ca3102b3d https://www.cpantesters.org/cpan/report/c77a5184-4d6f-11e8-9bc9-021ad4cd031b https://www.cpantesters.org/cpan/report/84c75978-5e44-11e8-bbaf-bdf0d2eb68fb

This is a bug in Log​::Agent\, but since it is long standing behavior in a non-core module\, I think it's reasonable to work around it in Storable. The optional use of Log​::Agent seems pretty questionable to me in general\, but since it has been there for as long as Storable has existed in core it seemed reasonable to leave it in place.

Removing (or disabling by default) the Log​::Agent integration appeals to me.

Perhaps it could be enabled with something like​:

  use Storable '-logagent';

or something similar.

The simplest workaround I could think of was to trap the error thrown by Log​::Agent\, then throw the original arguments given to our logcroak.

The patch isn't pretty\, but I think it's the only fix for this.

Tony

[p5pRT]

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

[p5pRT]

From @xsawyerx

On Wed\, 30 May 2018 17​:25​:10 -0700\, tonyc wrote​:

On Mon\, 28 May 2018 06​:27​:54 -0700\, haarg wrote​:

Storable rethrows errors from its hooks\, STORABLE_freeze\, STORABLE_thaw\, and STORABLE_attach. If those errors are refs\, they should be preserved in their original form. Tests were added to ensure this in 06f586d. However\, when Log​::Agent is installed\, Storable uses it rather than just calling Carp​::croak. Log​::Agent's logcroak function always stringifies the errors it gets when it throws them. This causes test failures in many cases when the dual life dist is installed from CPAN.

A few CPAN Testers reports from this issue​: https://www.cpantesters.org/cpan/report/00761375-6c0c-1014-946e- 8eaf8038f308 https://www.cpantesters.org/cpan/report/8e51c42a-4cf5-11e8-b010- a35ca3102b3d https://www.cpantesters.org/cpan/report/c77a5184-4d6f-11e8-9bc9- 021ad4cd031b https://www.cpantesters.org/cpan/report/84c75978-5e44-11e8-bbaf- bdf0d2eb68fb

This is a bug in Log​::Agent\, but since it is long standing behavior in a non-core module\, I think it's reasonable to work around it in Storable. The optional use of Log​::Agent seems pretty questionable to me in general\, but since it has been there for as long as Storable has existed in core it seemed reasonable to leave it in place.

Removing (or disabling by default) the Log​::Agent integration appeals to me.

Perhaps it could be enabled with something like​:

use Storable '-logagent';

or something similar.

The simplest workaround I could think of was to trap the error thrown by Log​::Agent\, then throw the original arguments given to our logcroak.

The patch isn't pretty\, but I think it's the only fix for this.

Tony

Patched applied. Resolving.

[p5pRT]

@xsawyerx - Status changed from 'open' to 'pending release'

[p5pRT]

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.30.0\, this and 160 other issues have been resolved.

Perl 5.30.0 may be downloaded via​: https://metacpan.org/release/XSAWYERX/perl-5.30.0

If you find that the problem persists\, feel free to reopen this ticket.

[p5pRT]

@khwilliamson - Status changed from 'pending release' to 'resolved'