Skip Menu |
 

This queue is for tickets about the POE CPAN distribution.

Report information
The Basics
Id: 34779
Status: rejected
Priority: 0/
Queue: POE

People
Owner: Nobody in particular
Requestors: nperez [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 1.0000
Fixed in: 1.0000



Subject: [PATCH] POE::Data::Envelope Integration Patch
Download (untitled) / with headers
text/plain 714b
Hola folks, I was wanting this to go in to the 1.0 release, but since it didn't make it not a big deal. Here is the promised integration patch with the POE distribution. It touches the shipped filters, tests, and ReadWrite to accommodate. I've add documentation to explain wtf is going on and also marked it as experimental. I've svk pulled from the trunk, and also tested the patch against a clean check out to verify that everything is kosher. And as always, if you see something that doesn't look right, please let me know. There very well may be follow up patches as I work on my own implementations based on this work and find something that doesn't work as planned. Comments welcome -- Nicholas R. Perez
Subject: PDE_Integration.patch

Message body is not shown because it is too large.

Download (untitled) / with headers
text/plain 1.2k
Hi, Nick! Now that it's the weekend, I have some time to look at your patch. It looks like a lot of tests have been altered to strip the blessing from results. Unfortunately tests like this don't test the actual data but rather an altered copy. I think that's bad form: is_deeply( - $next, [ "((($compare)))" ], + [@$next], [ "((($compare)))" ], "map filter get_one() returns ((($compare)))" ); This is significantly better, assuming it works: is_deeply( - $next, [ "((($compare)))" ], + $next, bless([ "((($compare)))" ], 'POE::Data::Envelope'), "map filter get_one() returns ((($compare)))" ); You've broken many of the ref() tests. ref([@$record]) is always equal to 'ARRAY' because you've wrapped the record in an arrayref! Worse, @$record will be a fatal error if ref($record) isn't based on ARRAY. my $records = $filter->get([ $get_request->as_string ]); - is(ref($records), 'ARRAY', 'simple get: get() returns list of requests'); + is(ref([@$records]), 'ARRAY', 'simple get: get() returns list of requests'); is(scalar(@$records), 1, 'simple get: get() returned single request'); As above, I'd like to see $records tested for what it is. ... and that's all I have time for today. I'll see about the filter/wheel changes tomorrow.
Subject: Re: [rt.cpan.org #34779] [PATCH] POE::Data::Envelope Integration Patch
Date: Sat, 19 Apr 2008 11:18:57 -0500
To: bug-POE [...] rt.cpan.org
From: "Nicholas Perez" <nicholasrperez [...] gmail.com>
Download (untitled) / with headers
text/plain 2.6k
Unfortunately, is_deeply, last time I looked, doesn't properly handle overloaded objects in the way they are meant to be handled, meaning the tests fail. Explicitly dereferencing the return result and wrapping it back up gets the test to pass. The goal when it came to the tests, was to modify as little as possible to make the tests pass. I didn't know how comfortable you would be with me rewriting all of the tests to test for these objects. If anything, I wanted to make sure the functionality was almost transparent. In all of the cases of dereferencing the result and wrapping it back up, it proves that the results obviously can be dereferenced successfully and that the meaning of the data is still the same, which covers the majority of the tests. And as for breaking ref tests, I didn't break them. Obviously if it fails to dereference, it is semantically still a failure, but I see your point. Now that I understand the allowed changes to the tests, I can go back and rewrite the tests to explicitly take the POE::Data::Envelope objects into account. Consider this patch a shoot from the hip largely because it has been ignored by everyone. On Sat, Apr 19, 2008 at 2:00 AM, RCAPUTO via RT <bug-POE@rt.cpan.org> wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=34779 > > > Hi, Nick! Now that it's the weekend, I have some time to look at your > patch. > > It looks like a lot of tests have been altered to strip the blessing > from results. Unfortunately tests like this don't test the actual data > but rather an altered copy. I think that's bad form: > > is_deeply( > - $next, [ "((($compare)))" ], > + [@$next], [ "((($compare)))" ], > "map filter get_one() returns ((($compare)))" > ); > > This is significantly better, assuming it works: > > is_deeply( > - $next, [ "((($compare)))" ], > + $next, bless([ "((($compare)))" ], 'POE::Data::Envelope'), > "map filter get_one() returns ((($compare)))" > ); > > You've broken many of the ref() tests. ref([@$record]) is always equal > to 'ARRAY' because you've wrapped the record in an arrayref! Worse, > @$record will be a fatal error if ref($record) isn't based on ARRAY. > > my $records = $filter->get([ $get_request->as_string ]); > - is(ref($records), 'ARRAY', 'simple get: get() returns list of > requests'); > + is(ref([@$records]), 'ARRAY', 'simple get: get() returns list of > requests'); > is(scalar(@$records), 1, 'simple get: get() returned single request'); > > As above, I'd like to see $records tested for what it is. > > ... and that's all I have time for today. I'll see about the > filter/wheel changes tomorrow. >
-- Nicholas R. Perez
Download (untitled) / with headers
text/plain 2.7k
On Wed Apr 09 02:49:23 2008, NPEREZ wrote: Show quoted text
> [patch]
I agree that the tests should verify that PDE behaves like plain old arrays for backward compatibility, but the spirit of many existing tests has been violated. I think the array-ness tests should be added, and the existing tests changed so their purposes remain intact. I'm looking at the modified POE::Filter::Stream. That's the most trivial filter, so your changes really stand out. The following are half-informed comments based on an incomplete understanding of your changes. Let me know where/how I get it wrong. Something's bugging me about the way POE::Data::Envelope objects hold data but are treated separate from it. This puts a constraint on filters: they must not pass two PDE objects amongst themselves, otherwise information is lost. So if a filter receives something which triggers two PDE objects, it will need to hold one of them until the next get_one() call. If two different PDE objects arrive via get_one_start() then the filter must not combine them: $f->get_one_start( PDE->new(...one...) ); $f->get_one_start( PDE->new(...two...) ); while (1) { my $one = $f->get_one(); last unless @$one; ... } In the above case, POE::Filter::Stream will lose the information encoded in the first PDE object. The second one will overwrite it. ... The get_one() call may not come for a while. For example, the line may be quiet, or select_pause_read() may have been called until the second PDE object arrives. In the latter case, there's a deadlock. sub get_one_start { if (blah blah) { # push two PDE objects into the stream } } So the wheels must, upon receipt of a PDE object, drive another filter get cycle just in case there's a pending object in the stack. ... The general get_one_start() logic may be something like this: If the new PDE is the same as the last one, then combine the data with the last one's. Otherwise push the PDE onto the stream buffer. A half-assed prototype: sub get_one_start { my ($self, $data) = @_; if (ref($data) eq ref($self->{buffer}[-1])) { push @{$self->{buffer}[-1]}, @$data; # or something similar } else { push @{$self->{buffer}}, $data; } } ... clone() clones the PDE object but not the data. I'm not sure that's a good idea, as it could be bad for PDE objects to bleed across filters. This could resolve itself if PDE objects were the buffer. ... POE::Filter's get() is not PDE aware. If get_one() returns a PDE object, get wraps it up in an anonymous array. ... Should get_pending() return the current buffer wrapped in the current PDE environment? POE::Wheel::Whatever feeds get_pending()'s return value to get_one_start() for the new filter. ... Probably other questions, but it's 4am. G'night!
Subject: Re: [rt.cpan.org #34779] [PATCH] POE::Data::Envelope Integration Patch
Date: Sun, 4 May 2008 20:01:25 -0500
To: bug-POE [...] rt.cpan.org
From: "Nicholas Perez" <nicholasrperez [...] gmail.com>
Download (untitled) / with headers
text/plain 4.1k
Very valid points. You're right. That is a hole that I didn't really consider, regarding two inputs into get_one_start, but it certainly makes sense if the data in one PDE isn't enough to trigger an event. And thinking about it more, perhaps for the core filters, it doesn't make sense to PDE enable them. The information passed around isn't enough to justify the extra rigmarole. And truly, a more complex protocol stack wouldn't end up using any of the core filters but would be more specialized. And to be honest, I did the most minimal integration into the core filters that I could. Since I am not using the core filters, I didn't have a use case to justify adding more than I needed to. The idea was to perhaps encourage others to add to them starting where I left off. And also if a core filter was used, the OOB data from upper levels in the stack wouldn't be lost. I appreciate the look-see, but I believe now I am going to retract the patch and instead implement a separate module that will bring this idea to fruition in a more specific context. On Sun, May 4, 2008 at 3:02 AM, RCAPUTO via RT <bug-POE@rt.cpan.org> wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=34779 > > > On Wed Apr 09 02:49:23 2008, NPEREZ wrote:
> > [patch]
> > I agree that the tests should verify that PDE behaves like plain old > arrays for backward compatibility, but the spirit of many existing tests > has been violated. I think the array-ness tests should be added, and > the existing tests changed so their purposes remain intact. > > I'm looking at the modified POE::Filter::Stream. That's the most > trivial filter, so your changes really stand out. > > The following are half-informed comments based on an incomplete > understanding of your changes. Let me know where/how I get it wrong. > > Something's bugging me about the way POE::Data::Envelope objects hold > data but are treated separate from it. > > This puts a constraint on filters: they must not pass two PDE objects > amongst themselves, otherwise information is lost. > > So if a filter receives something which triggers two PDE objects, it > will need to hold one of them until the next get_one() call. > > If two different PDE objects arrive via get_one_start() then the filter > must not combine them: > > $f->get_one_start( PDE->new(...one...) ); > $f->get_one_start( PDE->new(...two...) ); > while (1) { > my $one = $f->get_one(); > last unless @$one; > ... > } > > In the above case, POE::Filter::Stream will lose the information encoded > in the first PDE object. The second one will overwrite it. > > ... > > The get_one() call may not come for a while. For example, the line may > be quiet, or select_pause_read() may have been called until the second > PDE object arrives. In the latter case, there's a deadlock. > > sub get_one_start { > if (blah blah) { > # push two PDE objects into the stream > } > } > > So the wheels must, upon receipt of a PDE object, drive another filter > get cycle just in case there's a pending object in the stack. > > ... > > The general get_one_start() logic may be something like this: If the > new PDE is the same as the last one, then combine the data with the last > one's. Otherwise push the PDE onto the stream buffer. > > A half-assed prototype: > > sub get_one_start { > my ($self, $data) = @_; > if (ref($data) eq ref($self->{buffer}[-1])) { > push @{$self->{buffer}[-1]}, @$data; # or something similar > } > else { > push @{$self->{buffer}}, $data; > } > } > > ... > > clone() clones the PDE object but not the data. I'm not sure that's a > good idea, as it could be bad for PDE objects to bleed across filters. > > This could resolve itself if PDE objects were the buffer. > > ... > > POE::Filter's get() is not PDE aware. If get_one() returns a PDE > object, get wraps it up in an anonymous array. > > ... > > Should get_pending() return the current buffer wrapped in the current > PDE environment? POE::Wheel::Whatever feeds get_pending()'s return > value to get_one_start() for the new filter. > > ... > > Probably other questions, but it's 4am. G'night! >
-- Nicholas R. Perez
Rejecting the patch since it was rescinded.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.