Bleadperl v5.27.3-34-gf6107ca24b breaks MLEHMANN/AnyEvent-HTTP-2.23.tar.gz

[p5pRT]

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

Searchable as RT132142$

[p5pRT]

From @andk

As usual, cudos to discoverer Slaven!

bisect

commit f6107ca24b4cf22dcf7fd69d65612ad718c48fca
Author: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Date: Wed Aug 30 22:35:17 2017 +0100

Strengthen weak refs when sorting in-place

diagnostics:
http://www.cpantesters.org/cpan/report/86732923

Perl Info

perl -V
-------
Summary of my perl5 (revision 5 version 27 subversion 4) configuration:
Commit id: ae59d0fba73f3a4b9f082a26fad09e6cb2553dd2
Platform:
osname=linux
osvers=4.9.0-2-amd64
archname=x86_64-linux
uname='linux k93msid 4.9.0-2-amd64 #1 smp debian 4.9.18-1 (2017-03-30) x86_64 gnulinux '
config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.27.3-41-gae59d0fba7/3c10 -Dmyhostname=k93msid -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Dlibswanted=cl pthread socket inet nsl gdbm dbm malloc dl ld sun m crypt sec util c cposix posix ucb BSD gdbm_compat -Uuseithreads -Uuselongdouble -DEBUGGING=none'
hint=recommended
useposix=true
d_sigaction=define
useithreads=undef
usemultiplicity=undef
use64bitint=define
use64bitall=define
uselongdouble=undef
usemymalloc=n
default_inc_excludes_dot=define
bincompat5005=undef
Compiler:
cc='cc'
ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
optimize='-O2'
cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
ccversion=''
gccversion='6.3.0 20170406'
gccosandvers=''
intsize=4
longsize=8
ptrsize=8
doublesize=8
byteorder=12345678
doublekind=3
d_longlong=define
longlongsize=8
d_longdbl=define
longdblsize=16
longdblkind=3
ivtype='long'
ivsize=8
nvtype='double'
nvsize=8
Off_t='off_t'
lseeksize=8
alignbytes=8
prototype=define
Linker and Libraries:
ld='cc'
ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/6/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
libc=libc-2.24.so
so=so
useshrplib=false
libperl=libperl.a
gnulibc_version='2.24'
Dynamic Linking:
dlsrc=dl_dlopen.xs
dlext=so
d_dlsymun=undef
ccdlflags='-Wl,-E'
cccdlflags='-fPIC'
lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector-strong'


Characteristics of this binary (from libperl):
Compile-time options:
HAS_TIMES
PERLIO_LAYERS
PERL_COPY_ON_WRITE
PERL_DONT_CREATE_GVSV
PERL_MALLOC_WRAP
PERL_OP_PARENT
PERL_PRESERVE_IVUV
PERL_USE_DEVEL
USE_64_BIT_ALL
USE_64_BIT_INT
USE_LARGE_FILES
USE_LOCALE
USE_LOCALE_COLLATE
USE_LOCALE_CTYPE
USE_LOCALE_NUMERIC
USE_LOCALE_TIME
USE_PERLIO
USE_PERL_ATOF
Built under linux
Compiled at Sep 8 2017 09:50:13
%ENV:
PERL="/tmp/basesmoker-reloperl-OiBV/bin/perl"
PERL5LIB=""
PERL5OPT=""
PERL5_CPANPLUS_IS_RUNNING="23384"
PERL5_CPAN_IS_RUNNING="23384"
PERL_CANARY_STABILITY_NOPROMPT="1"
PERL_MM_USE_DEFAULT="1"
PERL_USE_UNSAFE_INC="1"
@INC:
/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.27.3-41-gae59d0fba7/3c10/lib/site_perl/5.27.4/x86_64-linux
/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.27.3-41-gae59d0fba7/3c10/lib/site_perl/5.27.4
/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.27.3-41-gae59d0fba7/3c10/lib/5.27.4/x86_64-linux
/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.27.3-41-gae59d0fba7/3c10/lib/5.27.4
.

[p5pRT]

From @ilmari

(Andreas J. Koenig) (via RT) <perlbug-followup@​perl.org> writes​:

bisect
------
commit f6107ca
Author​: Dagfinn Ilmari Mannsåker <ilmari@​ilmari.org>
Date​: Wed Aug 30 22​:35​:17 2017 +0100

Strengthen weak refs when sorting in\-place

diagnostics
-----------
http​://www.cpantesters.org/cpan/report/86732923

AnyEvent is relying on in-place sorting of its array of pending timers
keeping the references weak, and when it doesn't, user code can't cancel
a timer by dropping its reference to it.

Weakening all the references in the array after sorting it fixes the
issue, but that only works for arrays where all the references are weak.
If an array has a mix of strong and weak references one would have to
record which references were weak before the sort, and then go through
the sorted array and re-weaken them afterwards.

With the old behaviour, getting the strengthening effect was trivial​:
one could assign the array to itself before or after the sort, or defeat
the in-place sort altogether with the =()= trick.

This makes me think that despite all the work that's gone into making
the in-place sort optimisation invisible, the weak ref special case
needs to be kept, i.e. the above commit reverted.

I still think sv_rvunweaken() is a useful API function, so the commit
that added that should be kept.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

[p5pRT]

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

[p5pRT]

From zefram@fysh.org

Dagfinn Ilmari Mannsaker wrote​:

With the old behaviour, getting the strengthening effect was trivial​:

With the new behaviour, one can fairly trivially get the
weakness-preserving effect by wrapping the possibly-weak references in
an extra layer of reference.

This makes me think that despite all the work that's gone into making
the in-place sort optimisation invisible, the weak ref special case
needs to be kept, i.e. the above commit reverted.

-1. The optimisation should be a pure optimisation. If the useful
behaviour of the old faulty optimisation were somehow not easily
reproduced, it should be made available by explicit means, not kept as
a funny special case of an operator that normally means something else.

-zefram

[p5pRT]

From @andk

Also affected​: MGRIMES/AnyEvent-Filesys-Notify-1.21.tar.gz

Result type​: intermittent failures. Up to v5.27.3-33-gae2cf9f629 I ran
make test in a loop for several minutes and produced only passes, no
fails. With v5.27.3-34-gf6107ca24b and later I could produce both passes
and fails, it was just a matter of running a dozen or so tests to
produce both kinds of result.

--
andreas

[p5pRT]

From @tonycoz

On Fri, Sep 22, 2017 at 03​:48​:18AM +0100, Zefram wrote​:

Dagfinn Ilmari Mannsaker wrote​:

With the old behaviour, getting the strengthening effect was trivial​:

With the new behaviour, one can fairly trivially get the
weakness-preserving effect by wrapping the possibly-weak references in
an extra layer of reference.

This makes me think that despite all the work that's gone into making
the in-place sort optimisation invisible, the weak ref special case
needs to be kept, i.e. the above commit reverted.

-1. The optimisation should be a pure optimisation. If the useful
behaviour of the old faulty optimisation were somehow not easily
reproduced, it should be made available by explicit means, not kept as
a funny special case of an operator that normally means something else.

I agree.

Tony

[p5pRT]

From @ilmari

Zefram <zefram@​fysh.org> writes​:

Dagfinn Ilmari Mannsaker wrote​:

With the old behaviour, getting the strengthening effect was trivial​:

With the new behaviour, one can fairly trivially get the
weakness-preserving effect by wrapping the possibly-weak references in
an extra layer of reference.

Attached is a patch for AnyEvent that does this for its pending timer
list. It's pretty straight-forward, but I'm not in the mood for facing
MLEHMANN's wrath over perl5-porters breaking his code by submitting it
to him.

[p5pRT]

From @ilmari

0001-Use-strong-references-to-weak-references-in-the-time.patch

From eb30dd3aab9c33204418c77f7f81d3acb6e5b3eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 26 Sep 2017 22:03:48 +0100
Subject: [PATCH] Use strong references to weak references in the @timer

Perl 5.27.4 fixes the in-place sort optimisation (@a = sort @a) to
strengthen references, like assignment should.

However, we need to keep the references to timers weak, so callers can
cancel them by dropping their reference.  To achieve this, make the
actual references in @timer strong references to weak references to the
value returned to the user, and filter out ones that have gone undef
when necessary.
---
 lib/AnyEvent/Loop.pm | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/lib/AnyEvent/Loop.pm b/lib/AnyEvent/Loop.pm
index fb9351a..f57eab0 100644
--- a/lib/AnyEvent/Loop.pm
+++ b/lib/AnyEvent/Loop.pm
@@ -202,22 +202,26 @@ sub one_event {
    # first sort timers if required (slow)
    if ($MNOW >= $need_sort) {
       $need_sort = 1e300;
-      @timer = sort { $a->[0] <=> $b->[0] } @timer;
+      @timer = sort { $$a->[0] <=> $$b->[0] } grep { defined $$_ } @timer;
+   }
+   else {
+      # shift off any cancelled timers at the start of the queue
+      shift @timer while @timer and not defined ${$timer[0]};
    }
 
    # handle all pending timers
-   if (@timer && $timer[0][0] <= $MNOW) {
+   if (@timer && ${$timer[0]}->[0] <= $MNOW) {
       do {
-         my $timer = shift @timer;
-         $timer->[1] && $timer->[1]($timer);
-      } while @timer && $timer[0][0] <= $MNOW;
+         my $timer = ${shift @timer};
+         $timer && $timer->[1] && $timer->[1]($timer);
+      } while @timer && ${$timer[0]}->[0] <= $MNOW;
 
    } else {
       # poll for I/O events, we do not do this when there
       # were any pending timers to ensure that one_event returns
       # quickly when some timers have been handled
       my ($wait, @vec, $fds)
-         = (@timer && $timer[0][0] < $need_sort ? $timer[0][0] : $need_sort) - $MNOW;
+         = (@timer && ${$timer[0]}->[0] < $need_sort ? ${$timer[0]}->[0] : $need_sort) - $MNOW;
 
       $wait = $wait < MAXWAIT ? $wait + ROUNDUP : MAXWAIT;
       $wait = 0 if @idle;
@@ -252,7 +256,7 @@ sub one_event {
       } elsif (AnyEvent::WIN32 && $fds && $! == AnyEvent::Util::WSAEINVAL) {
          # buggy microshit windoze asks us to route around it
          CORE::select undef, undef, undef, $wait if $wait;
-      } elsif (!@timer || $timer[0][0] > $MNOW && !$fds) {
+      } elsif (!@timer || ${$timer[0]}->[0] > $MNOW && !$fds) {
          $$$_ && $$$_->() for @idle = grep $$$_, @idle;
       }
    }
@@ -319,8 +323,8 @@ sub timer($$$) {
    if ($interval) {
       $self = [$MNOW + $after , sub {
          $_[0][0] = List::Util::max $_[0][0] + $interval, $MNOW;
-         push @timer, $_[0];
-         weaken $timer[-1];
+         weaken(my $weak = $_[0]);
+         push @timer, \$weak;
          $need_sort = $_[0][0] if $_[0][0] < $need_sort;
          &$cb;
       }];
@@ -328,8 +332,8 @@ sub timer($$$) {
       $self = [$MNOW + $after, $cb];
    }
 
-   push @timer, $self;
-   weaken $timer[-1];
+   weaken(my $weakself = $self);
+   push @timer, \$weakself;
    $need_sort = $self->[0] if $self->[0] < $need_sort;
 
    $self
-- 
2.7.4

[p5pRT]

From @ilmari

This makes me think that despite all the work that's gone into making
the in-place sort optimisation invisible, the weak ref special case
needs to be kept, i.e. the above commit reverted.

-1. The optimisation should be a pure optimisation. If the useful
behaviour of the old faulty optimisation were somehow not easily
reproduced, it should be made available by explicit means, not kept as
a funny special case of an operator that normally means something else.

Normally I would agree, but the previous behaviour has been there as
long as in-place sort has, i.e. since 5.10. I also noticed that the
in-place reverse optimisation (since 5.12) also preservers weakness.
Presumably we want to fix that too (as I've done on the
ilmari/unweaken-inplace-reverse branch). Another "inconsistency" is
that in-place reverse preserves the deletedness of array elements.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article. - Calle Dybedahl

[p5pRT]

From zefram@fysh.org

Dagfinn Ilmari Mannsaker wrote​:

  It's pretty straight\-forward\, but I'm not in the mood for facing

MLEHMANN's wrath over perl5-porters breaking his code by submitting it
to him.

You could just stick it in the RT queue. (And face even more wrath.)

Normally I would agree, but the previous behaviour has been there as
long as in-place sort has, i.e. since 5.10.

It's been a bug all that time.

in-place reverse optimisation (since 5.12) also preservers weakness.
Presumably we want to fix that too

Yes.

                                     Another "inconsistency" is

that in-place reverse preserves the deletedness of array elements.

That's not detectable, is it? Preserving deletedness seems like it
would just avoid allocating some unnecessary SVs.

-zefram

[p5pRT]

From @ilmari

Zefram <zefram@​fysh.org> writes​:

Dagfinn Ilmari Mannsaker wrote​:

                                     Another "inconsistency" is

that in-place reverse preserves the deletedness of array elements.

That's not detectable, is it? Preserving deletedness seems like it
would just avoid allocating some unnecessary SVs.

$ perl -E '@​a=(1..3); delete $a[1]; @​a = reverse @​a; say 0+exists $a[1]'
0

--
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article. - Calle Dybedahl

[p5pRT]

From @andk

What is the state of this ticket? From looking at it it looks stalled.
Was there any additional activity?

Has anybody summarised? Reported to Marc? Provided a patch? Considered
improving? Considered reverting?

Thanks!
--
andreas

[p5pRT]

From @ilmari

Andreas Koenig <andreas.koenig.7os6VVqR@​franz.ak.mind.de> writes​:

What is the state of this ticket? From looking at it it looks stalled.
Was there any additional activity?

Has anybody summarised? Reported to Marc? Provided a patch? Considered
improving? Considered reverting?

I provided a patch in
https://rt.perl.org/Public/Bug/Display.html?id=132142#txn-1493873, but
just realised there's a simpler way to do it, which I've attached.

As mentioned in that previous message, I'm not in the mood to confront
Marc over this, but Sawyer graciously offered to do that for me.

Sawyer, do you feel like sending these two patches to Marc and suggest
he applies one of them to keep AnyEvent working on blead? Note that
AnyEvent's own test suite doesn't trigger the bug, but AnyEvent​::HTTP
does.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article. - Calle Dybedahl

[p5pRT]

From @ilmari

0001-Re-weaken-timer-references-after-sorting.patch

From 8e12a666543e50043419630d5e4ff00c7dd277e2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 17 Nov 2017 23:59:30 +0000
Subject: [PATCH] Re-weaken timer references after sorting

Perl 5.27.4 fixes the in-place sort optimisation (@a = sort @a) to
strengthen references, like assignment should.

However, we need to keep the references to timers weak, so callers can
cancel them by dropping their reference.
---
 lib/AnyEvent/Loop.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/AnyEvent/Loop.pm b/lib/AnyEvent/Loop.pm
index fb9351a..f7145fd 100644
--- a/lib/AnyEvent/Loop.pm
+++ b/lib/AnyEvent/Loop.pm
@@ -173,6 +173,9 @@ BEGIN {
    $round = 0.001 if $round < 0.001; # 1ms is enough for us
    $round -= $round * 1e-2; # 0.1 => 0.099
    eval "sub ROUNDUP() { $round }";
+
+   my $unweakens = $] >= 5.027004 ? 1 : 0;
+   eval "sub INPLACE_SORT_UNWEAKENS() { $unweakens }";
 }
 
 _update_clock;
@@ -203,6 +206,9 @@ sub one_event {
    if ($MNOW >= $need_sort) {
       $need_sort = 1e300;
       @timer = sort { $a->[0] <=> $b->[0] } @timer;
+      if (INPLACE_SORT_UNWEAKENS) {
+          weaken($_) for @timer;
+      }
    }
 
    # handle all pending timers
-- 
2.7.4

[p5pRT]

From @xsawyerx

On Fri, 17 Nov 2017 16​:09​:12 -0800, ilmari wrote​:

Andreas Koenig <andreas.koenig.7os6VVqR@​franz.ak.mind.de> writes​:

What is the state of this ticket? From looking at it it looks stalled.
Was there any additional activity?

Has anybody summarised? Reported to Marc? Provided a patch? Considered
improving? Considered reverting?

I provided a patch in
https://rt.perl.org/Public/Bug/Display.html?id=132142#txn-1493873, but
just realised there's a simpler way to do it, which I've attached.

As mentioned in that previous message, I'm not in the mood to confront
Marc over this, but Sawyer graciously offered to do that for me.

Sawyer, do you feel like sending these two patches to Marc and suggest
he applies one of them to keep AnyEvent working on blead? Note that
AnyEvent's own test suite doesn't trigger the bug, but AnyEvent​::HTTP
does.

I didn't receive an email for this correspondence so I only saw the ping from Andreas. I have just sent an email to Marc with the latest patch, asking to apply the patch and release a new version. P5P was CC'ed on the email as well.

Thank you for producing the patches, Ilmari.