Skip Menu |

This queue is for tickets about the IO-KQueue CPAN distribution.

Report information
The Basics
Id: 61481
Status: open
Priority: 0/
Queue: IO-KQueue

People
Owner: Nobody in particular
Requestors: leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Missing SvREFCNT_dec() ?
Hi there, I'm looking into writing an IO::Async loop for BSD machines[1], based on IO::KQueue (analogous to way IO::Async::Loop::Epoll[2] uses IO::Epoll[3]). Having had a skim-read of the .xs file used to implement IO::KQueue, I can't see an associated SvREFCNT_dec() call to go alongside the _inc() one used to adjust the refcount of an SV* stored in the udata field of a kevent. On a more careful reading, it appears that it's SvREFCNT_inc()'ing the actual passed SV, rather than, say, taking a copy of it. There's a number of awkward upshots from this; consider for example: my $data; foreach my $fh ( @set_of_filehandles ) { $data = { filehandle => $fh }; $kq->EV_SET( $fh->fileno, EVFLT_READ, EV_ADD, 0, $data ); } What you've now done is stored that $data lexical lots of times, per filehandle, and all the stored HASHrefs happen to point at the last set value in the for loop. I'd also expect that the kqueue object itself can hold on to a reference itself. It happens to do that currently, by an accident of the SvREFCNT_inc applied at the wrong level. Instead I'd suggest that it stores a new SV created by newSVsv, which will copy temporary numbers/strings/etc.., and automatically SvREFCNT_inc a reference. ----- [1]: https://rt.cpan.org/Ticket/Display.html?id=61478 [2]: http://search.cpan.org/~pevans/IO-Async-Loop-Epoll-0.07/ [3]: http://search.cpan.org/~brucek/IO-Epoll-0.02/lib/IO/Epoll.pm -- Paul Evans
Can you send a patch?
On Fri Oct 01 09:55:31 2010, MSERGEANT wrote: Show quoted text
> Can you send a patch?
Short answer: No, I cannot. Long answer: Not really. I could give you a change to clone SVs and store those, rather than storing SvREFCNT_inc()'ed pointers to existing ones. That's not hard. Problem is, that misses the real fundamental problem that this would cause a memory leak, exactly as it currently causes a memory leak. You'd need to ensure you SvREFCNT_dec()'ed the SV pointers whenever kevents get removed. E.g. if someone calls EV_SET() EV_DELETE, or if the event had been EV_ONESHOTed, or if it was watching a filehandle that has now been close()ed, or a child process that has now exit()ed. In each of these cases, the kernel will now remove its kevent watching structure, so we must SvREFCNT_dec() the pointer to allow it to free if required. Furthermore, from the look of things, when you want to EV_DELETE, the kernel will not pass back up the SV* in udata, so it means that IO::KQueue is going to have to store another copy of all these SV*s anyway, so it can SvREFCNT_dec() them on its own. It's all starting to get more messy now.... Perhaps this is something we can discuss in more detail, perhaps on IRC? I'm "LeoNerd" on Freenode and irc.perl.org. -- Paul Evans
On Tue Oct 05 17:03:41 2010, PEVANS wrote: Show quoted text
> On Fri Oct 01 09:55:31 2010, MSERGEANT wrote:
> > Can you send a patch?
> > Short answer: No, I cannot. > > Long answer: > > Not really. I could give you a change to clone SVs and store those, > rather than storing SvREFCNT_inc()'ed pointers to existing ones. > That's not hard.
Find attached a patch for this particular part. I also went and actually added some unit tests of basic READ SIGNAL and TIMER event watchers, since there were none. The greater problem of when to eventually SvREFCNT_dec() these values still remains. I suspect it may not be easily possible in general, but perhaps some attack on the problem can still be made, without complicating the code too much. I've now joined the freebsd-hackers@ mailing list; I'll post a question there and see what the response is. -- Paul Evans
On Thu Nov 11 19:42:06 2010, PEVANS wrote: Show quoted text
> Find attached a patch for this particular part.
Uh.. I fail. _now_ attached. -- Paul Evans
Subject: rt-61481.diff
=== modified file 'KQueue.xs' --- KQueue.xs 2010-11-11 19:12:54 +0000 +++ KQueue.xs 2010-11-11 21:34:07 +0000 @@ -56,11 +56,7 @@ int i; PPCODE: memset(&ke, 0, sizeof(struct kevent)); - if (udata) - SvREFCNT_inc(udata); - else - udata = &PL_sv_undef; - EV_SET(&ke, ident, filter, flags, fflags, data, udata); + EV_SET(&ke, ident, filter, flags, fflags, data, udata ? newSVsv(udata) : NULL); i = kevent(kq, &ke, 1, NULL, 0, NULL); if (i == -1) { croak("set kevent failed: %s", strerror(errno)); @@ -107,7 +103,7 @@ av_push(array, newSViv(ke[i].flags)); av_push(array, newSViv(ke[i].fflags)); av_push(array, newSViv(ke[i].data)); - av_push(array, SvREFCNT_inc(ke[i].udata)); + av_push(array, ke[i].udata ? newSVsv(ke[i].udata) : newSV(0)); PUSHs(sv_2mortal(newRV_noinc((SV*)array))); } === modified file 'Makefile.PL' --- Makefile.PL 2010-11-11 19:12:54 +0000 +++ Makefile.PL 2010-11-11 21:19:44 +0000 @@ -20,7 +20,10 @@ WriteMakefile( VERSION_FROM => 'KQueue.pm', NAME => 'IO::KQueue', - PREREQ_PM => { }, + PREREQ_PM => { + 'Test::Identity' => 0, + 'Test::Refcount' => 0, + }, ABSTRACT_FROM => 'KQueue.pm', AUTHOR => 'Matt Sergeant <matt@sergeant.org>', clean => {FILES => "tv.log"} === added file 't/10read.t' --- t/10read.t 1970-01-01 00:00:00 +0000 +++ t/10read.t 2010-11-11 21:35:44 +0000 @@ -0,0 +1,52 @@ +#!/usr/bin/perl -w + +use strict; +use warnings; + +use Test::More tests => 11; +use Test::Identity; +use Test::Refcount; + +use IO::KQueue; + +use IO::Handle; + +my $kq = IO::KQueue->new; + +pipe( my $readh, my $writeh ) or die "Cannot pipe() - $!"; + +my $userdata = [ "user data" ]; +is_oneref( $userdata, '$userdata has one reference initially' ); + +$kq->EV_SET( fileno($readh), EVFILT_READ, EV_ADD, 0, 0, $userdata ); + +is_refcount( $userdata, 2, '$userdata has two references after ->EV_SET(EV_ADD)' ); + +my @events; +@events = $kq->kevent( 0 ); + +is( scalar @events, 0, 'No events initially' ); + +$writeh->syswrite( "Hello\n" ); + +@events = $kq->kevent( 1 ); + +is( scalar @events, 1, 'One event received' ); + +my $ev = shift @events; +is( $ev->[KQ_IDENT], fileno $readh, '$ev->[KQ_IDENT]' ); +is( $ev->[KQ_FILTER], EVFILT_READ, '$ev->[KQ_FILTER]' ); +is( $ev->[KQ_FLAGS], 0, '$ev->[KQ_FLAGS]' ); +is( $ev->[KQ_DATA], 6, '$ev->[KQ_DATA]' ); +identical( $ev->[KQ_UDATA], $userdata, '$ev->[KQ_UDATA]' ); + +is_refcount( $userdata, 3, '$userdata has three references after ->kevent' ); + +# Clear it +$readh->sysread( my $buffer, 8192 ); + +undef $ev; + +$kq->EV_SET( fileno($readh), EVFILT_READ, EV_DELETE ); + +is_oneref( $userdata, '$userdata has one reference finally' ); === added file 't/11signal.t' --- t/11signal.t 1970-01-01 00:00:00 +0000 +++ t/11signal.t 2010-11-11 21:35:50 +0000 @@ -0,0 +1,59 @@ +#!/usr/bin/perl -w + +use strict; +use warnings; + +use Test::More tests => 13; +use Test::Identity; +use Test::Refcount; + +use IO::KQueue; + +use POSIX qw( SIGUSR1 ); + +my $kq = IO::KQueue->new; + +$SIG{USR1} = "IGNORE"; + +my $userdata = [ "user data" ]; +is_oneref( $userdata, '$userdata has one reference initially' ); + +$kq->EV_SET( SIGUSR1, EVFILT_SIGNAL, EV_ADD, 0, 0, $userdata ); + +is_refcount( $userdata, 2, '$userdata has two references after ->EV_SET(EV_ADD)' ); + +my @events; +@events = $kq->kevent( 0 ); + +is( scalar @events, 0, 'No events initially' ); + +kill USR1 => $$; + +@events = $kq->kevent( 1 ); + +is( scalar @events, 1, 'One event received' ); + +my $ev = shift @events; +is( $ev->[KQ_IDENT], SIGUSR1, '$ev->[KQ_IDENT]' ); +is( $ev->[KQ_FILTER], EVFILT_SIGNAL, '$ev->[KQ_FILTER]' ); +is( $ev->[KQ_FLAGS], EV_CLEAR, '$ev->[KQ_FLAGS]' ); +is( $ev->[KQ_DATA], 1, '$ev->[KQ_DATA]' ); +identical( $ev->[KQ_UDATA], $userdata, '$ev->[KQ_UDATA]' ); + +kill USR1 => $$; +kill USR1 => $$; + +@events = $kq->kevent( 1 ); + +is( scalar @events, 1, 'One event received after two kill()s' ); + +$ev = shift @events; +is( $ev->[KQ_DATA], 2, '$ev->[KQ_DATA] == 2' ); + +is_refcount( $userdata, 3, '$userdata has three references after ->kevent' ); + +undef $ev; + +$kq->EV_SET( SIGUSR1, EVFILT_SIGNAL, EV_DELETE ); + +is_oneref( $userdata, '$userdata has one reference finally' ); === added file 't/12timer.t' --- t/12timer.t 1970-01-01 00:00:00 +0000 +++ t/12timer.t 2010-11-11 21:36:11 +0000 @@ -0,0 +1,43 @@ +#!/usr/bin/perl -w + +use strict; +use warnings; + +use Test::More tests => 11; +use Test::Identity; +use Test::Refcount; + +use IO::KQueue; + +my $kq = IO::KQueue->new; + +my $userdata = [ "user data" ]; +is_oneref( $userdata, '$userdata has one reference initially' ); + +$kq->EV_SET( 1, EVFILT_TIMER, EV_ADD, 0, 50, $userdata ); + +is_refcount( $userdata, 2, '$userdata has two references after ->EV_SET(EV_ADD)' ); + +my @events; +@events = $kq->kevent( 0 ); + +is( scalar @events, 0, 'No events initially' ); + +@events = $kq->kevent( 100 ); + +is( scalar @events, 1, 'One event received' ); + +my $ev = shift @events; +is( $ev->[KQ_IDENT], 1, '$ev->[KQ_IDENT]' ); +is( $ev->[KQ_FILTER], EVFILT_TIMER, '$ev->[KQ_FILTER]' ); +is( $ev->[KQ_FLAGS], EV_CLEAR, '$ev->[KQ_FLAGS]' ); +is( $ev->[KQ_DATA], 1, '$ev->[KQ_DATA]' ); +identical( $ev->[KQ_UDATA], $userdata, '$ev->[KQ_UDATA]' ); + +is_refcount( $userdata, 3, '$userdata has three references after ->kevent' ); + +undef $ev; + +$kq->EV_SET( 1, EVFILT_TIMER, EV_DELETE ); + +is_oneref( $userdata, '$userdata has one reference finally' );
On Thu Nov 11 19:42:06 2010, PEVANS wrote: Show quoted text
> I've now joined the freebsd-hackers@ mailing list; I'll post a question > there and see what the response is.
http://lists.freebsd.org/pipermail/freebsd-hackers/2010- November/033565.html watch this space... -- Paul Evans
On Fri Nov 12 15:10:39 2010, PEVANS wrote: Show quoted text
> On Thu Nov 11 19:42:06 2010, PEVANS wrote: >
> > I've now joined the freebsd-hackers@ mailing list; I'll post a question > > there and see what the response is.
> > http://lists.freebsd.org/pipermail/freebsd-hackers/2010- > November/033565.html
Well, over three years on and no response yet. I raised a PR at http://www.freebsd.org/cgi/query-pr.cgi?pr=153254 and I've recently re-posted it to this thread http://lists.freebsd.org/pipermail/freebsd-hackers/2014-March/044463.html -- Paul Evans