Re: POE::Filter error reporting

[Rocco Caputo]

Yay, my outbound e-mail works again!  I can comment on this thread now!
I require three things (today... maybe more, or others tomorrow):

1. Maintain backwards compatibility.  Old and new filters should  
interoperate.  Require the least amount of collateral code change  
possible.  Most of the burden of this feature is implemented in the  
filters themselves.

2. Filters must work stand-alone.  This is a great feature, allowing  
people to write thin, blocking clients to POE servers.  The technique  
is documented in at least one cookbook example.

3. Filter status notifications must remain in-band.  That is, it  
should be passed through filters along with regular data.  This  
should resolve a potential race condition between the last data in  
some stream and the EOF itself.  Currently EOF is delivered  
immediately upon the driver noticing it.  If any data remains in the  
filter pipeline, it's lost.

The typed container idea works for me.  If we require the get_one()  
interface, it already returns an untyped container: the list  
reference that envelopes the message.  Normal data can leave the  
envelope type unblessed.  Various statuses can be defined by  
POE::Filter::Datatype subclasses and bound to the envelope with bless().

There would need to be a way for external glue code (POE::Wheel::*  
and POE::Filter::Stackable, mainly) to tell whether a filter supports  
in-band notifications, without modifying the filters that don't.   
Possibly some accessor that's present on the newer filters.

In-band notifications would require filter buffers to be list-based.   
Notifications must remain separate from data within the filters.   
Some filters, like POE::Filter::Stream, have scalar-based buffers.

In-band notifications would require the get_one() interface.  get_one 
() returns data wrapped in superfluous listref envelopes.  We can  
bind type information to that envelope without touching the messages  
that pass through the filter.

The API for get_pending() may need to change. :(

Filter::Stackable would check each filter in the stack.  Status  
notifications would skip filters that don't support them.

POE::Filter::Datatype types would not be exposed to the application.   
They are only used internally by POE::Wheel::* and the filters.   
Convert them to "plain" messages (move the status inside the  
envelope, and clear the envelope type) if you want to expose them to  
the user.  Or wheels can catch them and do something with them.  For  
example, POE::Filter::Datatype::EOF would be converted to an  
ErrorEvent by POE::Wheel::ReadWrite.  It would become a ClosedEvent  
in POE::Wheel::Run.

Some sample code, adapted from POE::Filter::Stream and not at all  
tested:

sub new {
  my $class = shift;
  return bless [ ], $class;
}

sub supports_inband_status { 1 }

sub get_one_start {
  my ($self, $stream) = @_;

  # If the $stream envelope has a special data type, then
  # distribute that across all the messages in the envelope.

  if (ref($stream) =~ /^POE::Filter::Datatype::/) {
    push @$self, map { [ ref($stream), $_ ] } @$stream;
    return;
  }

  # Otherwise it's plain data.  Combine the streamed data,
  # and buffer it.

  if (@$self and ref($self->[-1]) eq "ARRAY") {
    $self->[-1] .= join "", @$stream;
    return;
  }

  push @$self, join "", @$stream;
  return;
}

# Return the next chunk, which was prepared in get_one_start().

sub get_one {
  my $self = shift;
  return bless [ ], "POE::Filter::Datatype::Empty" unless @$self;
  return shift @$self;
}

# Return a list rather than a list reference.
# The problem here is that we may be returning
# many different types of things, each requiring
# its own envelope.

sub get_pending {
  my $self = shift;
  return @$self;
}

--
Rocco Caputo - [EMAIL PROTECTED]


On Aug 15, 2007, at 11:57, Nicholas Perez wrote:

The only reason I questioned it is because I heard mentioned in #poe
about wanting to maintain the non-POE-ness of filters (ie. usage
outside of POE). I could have the context wrong, though and I forgot
specifically who said it? Chris maybe?

On 8/15/07, Matt Sickler <[EMAIL PROTECTED]> wrote:
How is adding that error mechanism binding the filter to the wheel  
any
more than the current get_one() and put() API is?

I don't care how many API's we have to break, Filters should have
proper error checking.
Sure there are alot of third party filters, but 90% of the devs  
are on
#poe, and the other 10% are probably smart enough to read the docs  
and
figure out the changes.

On 8/15/07, Nicholas Perez <[EMAIL PROTECTED]> wrote:
This certainly looks promising, but how do you know when to read it?
When the filter returns an empty array ref? I'd like not for the  
error
checking to be required after every time an empty array ref gets
returned, because that could be quite a few times depending on  
how big
of a buffer the filter has before a usable chunk comes out the other
end. Also, it seems like the OOB stuff shouldn't get passed in to
get_one(). While I'm all for breaking the API to fix something, it
seems like there should be an OOB mechanism built-in, that doesn't
require passing arguments to get_one.

I still thinking adding a property is the way to go. To indicate  
error
state, I like the idea of passing into the constructor a ref to a
scalar and checking its boolean value after each execution (I think
this sounds like a compromise). That seems like less cycles for  
error
checking, and we don't change the return [ ] behavior on error or no
records.

The only problem with an error indicator passed in is this: we have
just coupled the container to the filter (as witnessed by the  
changes
to readwrite). Do we really want this?

In reality any of these approaches will work. I would just like to
find the least painful of them all.

On 8/14/07, David Davis <[EMAIL PROTECTED]> wrote:
My work on POE::Filter::SSL is relevant here.

I needed a way to pass OOB data back to the readwrite wheel  
during the SSL
negotiation.  The method LotR and I came up works great.  I pass  
an array
ref to get_one (lets call it $oob), and any time a filter has  
OOB data, it
blesses it into a namespace, and pushes it onto @$oob.  For  
Filter::SSL, I
used the OOB namespace 'POE::OOB::put' for data that needs to be  
directly
sent out the wheel's driver.  Filter::Stackable and  
Wheel::ReadWrite needs
to be aware of this OOB data, so I've patched them too.  We can  
use this
method to add 'POE::OOB::error' and handle it by sending an event.

Relevant files:
http://svn.xantus.org/sprocket/trunk/Sprocket/lib/POE/Filter/ 
Stackable.pm
http://svn.xantus.org/sprocket/trunk/Sprocket/lib/POE/Filter/SSL.pm
http://svn.xantus.org/sprocket/trunk/Sprocket/lib/POE/Wheel/ 
ReadWrite.pm

Cheers,
David

On 8/14/07, Martijn van Beers <[EMAIL PROTECTED]> wrote:

There was a discussion about this on #poe, which I thought I'd  
share:

<+nperez> an isa check every single time something goes through  
the
          filter just seems like overkill...
<+LotR> compared to the work that Filter::XML does?
<+nperez> yes, because it will add up. hold on, writing a  
benchmark
script
<+LotR> I'm sure it will prove to be nothing compared to  
parsing an xml
        subtree
<+nperez> but what about other filters? Stream? Line?
<+LotR> stream doesn't ever need to generate any errors. nor  
does line
<+LotR> if you just use those, you can forget about error checking
        altogether
* LotR will send the discussion to the list when we're done
<+nopaste> "nperez" at 12.5.72.252 pasted "undef vs isa" (30  
lines) at
           http://nopaste.snit.ch:8001/10886
<+nperez> take a look
<+nperez> that's a huge difference
<+LotR> but an isa() still is pretty damn fast.
<+nperez> besides, it is less of a rewrite checking for undef.
consider:
          you have to now check ref and then isa on every  
return value
          from the filter
<+nperez> what if it isn't a ref that gets returned?
<+nperez> and isa only worked on blessed objects
<+LotR> you're still going to have to check for undef too
<+nperez> works
<+nperez> exactly, so why do the other checks and you can just  
do the
one
<+LotR> and it is going to be more of a change if you then have  
to ask
        the filter for extra data
<+nperez> not really, implementing an get_error() isn't as  
profound of
a
          change
<+nperez> it is OOB
<+nperez> without you still have checking for undef for errors  
in a lot
          of modules anyhow
<+nperez> if they want to know more info, they then call  
get_error()
<+nperez> otherwise, they might not care
<+LotR> that "whout you still.." sentence doesn't parse for me
<+LotR> without. duh
<+nperez> sorry, without get_error(), you still have checking  
for undef
<+LotR> still don't get it. how does checking for undef for  
errors in
'a
        lot of modules' have anything to do with filters?
<+nperez> because right now, the de facto indicator for error is
          returning undef
<+LotR> no it isn't
<+LotR> in Filter::HTTPD, an HTTP::Response is returned
<+LotR> in Filter::XML, you get a callback
<+LotR> in Filter::SAXBuilder, you get an Error::Simple object
<+LotR> and I don't know any other filters that return errors  
right now
<+nperez> okay, thinking a little more general, perl built-ins  
return
          undef on errors and set $!
<+nperez> following that convention can't be /that/ bad
<+LotR> right, but is it guaranteed that you always get the  
chance to
        check the error before an extra error is created (meaning
filters
        have to store more than one, and the $! analogy breaks  
down)
<@Leolo> test it against Scalar::Util::blessed
<+nperez> Leolo: that is a possibility too
<+LotR> Leolo: hmm?
<@Leolo> also, the filter could return undef(), then you ask  
the filter
         ->is_error()  or ->error_str() or whatever
<@Leolo> call this wrapped in POE::Filter::Fallible
<+LotR> I just don't particulary like the extra API
<@Leolo> s/call/all/
<+nperez> well, it's incomplete as is. Error states are simply
          unaccounted for. Most of the modules that I have been  
looking
          through in the past 10 minutes make all sorts of  
assumptions
on
          input
<+nperez> if there is an error, it will just die
<+nperez> or return [ ]
<+nperez> in which case, how do you know there was an error?
<+LotR> you don't. But I like the simplicity of array ref in;  
array ref
        out
<@Leolo> they probably assume they are talking to another
         POE::Filter::Foo.  in which case there should be no error
<@Leolo> lotr : true
<+nperez> the simplicity fails. If the filter gets into an  
inconsistent
          state due to bad input, but never tells the outside  
world
what
          is going on, that's just poor
<+nperez> returning [] is just too overloaded
<+LotR> plus, if you stack the filters, can you make sure you  
will just
        have had one error if you get out of the filter?
<+LotR> nperez: I'm not saying returning [] on error is a good  
idea, it
        isn't.
<+nperez> there probably needs to be a way to aggregate/bubble  
errors
up
          from stacked filters
<+LotR> I'm saying returning an error object in the array ref  
out keeps
        the simplicity
< imMute> maybe it should return some kind of  
POE::Filter::Error object
          on error
< imMute> and then the wheel thats processing it can deal with  
the error
< imMute> or the next filter in stackable can just pass it on
<+nperez> LotR: but that adds checking in-band which is a  
performance
          killer
<+nperez> maybe no killer, but detrimental to performance i  
should say
<+nperez> maybe no killer, but detrimental to performance i  
should say
< imMute> its better than just suddenly not getting any records  
from a
          filter and not knowing why
<+LotR> nperez: right. so we disagree on how bad such a  
performance hit
        that is
<+nperez> imMute: getting an undef should be a strong idicator  
of error
          in which case you check the error property on the filter
<+nperez> agreed on that point LotR
< imMute> nperez: are we going to standardize the name of that  
call,
and
          what it should return?
* LotR wonders if there are no filters that return undef for  
reasons
    other than error
<+nperez> imMute: that's what we would like to happen, yes
< imMute> personally I think we should standardize the arrayref  
in,
          arrayref out  first, because I know there are filters  
that
dont
          follow that
<+nperez> it is standard, if they subclass POE::Filter
<+nperez> and stay "true" to form
< imMute> some dont though
<+nperez> then we should file bugs on rt for them :)
< imMute> i know few of BinGOs' filters subclass POE::Filter
< imMute> and he complained loudly when i added an isa() check to
          Stackable
<@BinGOs> all of them do.
<+nperez> I did too
<+nperez> I had to make changes to make the filter stabled
<+nperez> stackable
<+purl> well, stackable is newer than PFX?
<@BinGOs> this is why I was screaming blue fucking murder earlier.
< imMute> maybe its changed since i last messed with filters,  
its been
a
          few months
<+nperez> It is implicit that filters take aref in and spit  
aref out
<+LotR> explicit in the docs actually
<+nperez> right
<@BinGOs> My take is POE::Filter shouldn't shit out anything they
          shouldn't
<+LotR> BinGOs: meaning?
<+purl> somebody said meaning was in the individual, whether a
definition
<@BinGOs> otherwise a world of fucking hurt is going to happen.
<@BinGOs> meaning, don't return anything on a fucking error.
<+LotR> why not?
<@BinGOs> because I will hunt you down like a dog
<+nperez> BinGOs: if there is an error, I'd like to know about it
<@BinGOs> then 'warn'
<+nperez> returning [] is not tenable
<+nperez> the state of the filter may need to be reset
<+nperez> I can't capture warns and adjust accordingly
<@BinGOs> if you are in a POE::Filter::Stackable then everyone  
may not
be
          following the rules.
<+nperez> which is why we are wanting to update the POE::Filter  
API and
          introduce a standard way to report errors
<+LotR> BinGOs: there'll be new rules :)
<+nperez> heh
<@BinGOs> I am not the bad guy here >:)
<+nperez> BinGOs: if you want patches to your modules after the  
change,
          I'll help, promise. Because you have a shit load and  
I don't
          blame you for beign resistant to change ;)
<+LotR> BinGOs: oh no? so what happens if Filter::IRC gets a  
line that
        doesn't parse?
<@BinGOs> it warns
<+LotR> which isn't very nice
<@BinGOs> and b). I am not using POE::Filter::IRC these days.
<+LotR> Filter::IRCD then
<@BinGOs> and c). yes, POE::Filter::IRCD warns
<+nperez> warnings disappear into the aether and don't control
          autocorrecting procedures
<+LotR> BinGOs: which you can't handle very nicely.
<@BinGOs> Okay, you have a POE::Filter::Stackable
< imMute> what does PCI use for the client then?
<@BinGOs> the first filter produces an error, what happens next ?
<@BinGOs> ( you have four filters in the chain )
<+nperez> stackable checks the error property, sets its own error
          property and returns undef
<+nperez> skipping the other three
<@BinGOs> do you produce an event at the end ?
<+LotR> or
<+LotR> it sticks an error object in the listref out. (and  
probably
skips
        the other three filters)
< imMute> BinGOs: no
<+LotR> BinGOs: filters don't know anything about POE events. I  
want to
        keep it that way
<@BinGOs> PCI uses Filter::IRCD and POE::Filter::IRC::Compat in a
          stackable.
< imMute> BinGOs: stackable doesnt produce output unless the last
filter
          returns something
<@BinGOs> SO what happens to your error then ?
<+nperez> returning undef should be an indicator to check the  
error
          property on the filter
<+LotR> BinGOs: read the scrollback? we've been over this and  
haven't
        come to an agreement. nperez and I have different views  
on how
to
        do it
<+LotR> but we agree there should be some error reporting in  
filters
<@BinGOs> Hell, I'll work around whatever you come up with  
anyways >:)
<+nperez> haha
<@BinGOs> difficult audience.
<+nperez> Ultimately is up to dngor about an API change
<+nperez> we are wanting to do something rather fundamental
<+nperez> especially with a 1.0 approaching
<@BinGOs> this is why I am stomping my little feet.
<+nperez> If it's broken, it's broken. Bitching how much the  
fix hurts
          isn't going to make it go away :)
<+LotR> another reason not to do an ->get_error() method. no  
need to
        change old filters who don't care about error reporting
<+nperez> if they don't care about error reporting, they aren't
checking
          their inputs enough ;)
<@BinGOs> I am a team player and have a 'Can do attitude'
<@BinGOs> the whole POE::Filter sub-class thing was no problem  
to me.
<+nperez> BinGOs: we aren't interviewing you here. You are more  
than
          welcome to be the devil's advocate and general  
asshole. It
          helps :)
<@BinGOs> Yes, I am playing devil's advocate
<@BinGOs> and yes, I am an asshole.
<+nperez> You represent a large swath of incumbent modules. Your
opinion
          counts
<@BinGOs> I always have an eye on backwards compatibility.
<@perigrin> to watch it slip away
<+nperez> That is certainly something to consider. LotR's idea is
          probably better for backwards compat
<@Leolo> backwards compatible with "do nothing useful" is easy





--

David Davis
Software Engineer