Re: POE::Component::Server::TCP bug fixes, possibly incompatible
I went with option C. The change is committed and ready for review: http://poe.svn.sourceforge.net/viewvc/poe?view=rev&revision=2613 The commit message reads: !!! This change breaks backward compatibility on a relatively unused !!! feature. You are affected if you use ARG0 or ARG1 in a !!! POE::Component::Server::TCP ClientConnected callback. ClientArgs promised more than it could deliver, and people finally noticed. This change backs off supplying the socket in $_[ARG0], and it expands ClientArgs' arrayref into @_[ARG0..$#_]. Thanks to Michael Fowler for rt.cpan.org #47855 (which this resolves), and POE's mailing list for advice on which way this change should go. -- Rocco Caputo - rcap...@pobox.com On Jul 21, 2009, at 06:40, Olivier Mengué wrote: Euh, well, I was meaning option A. But either A or C is good for me. Option B is too awkward as a bad API would stay forever and would bite any new Server::TCP user. Le 21 juillet 2009 12:37, Olivier Mengué a écrit : 2009/7/16 Chris 'BinGOs' Williams Anyways, I see three options: a). Make the functionality match the documentation; b). Make the documentation match the functionality; c). Do what dngor suggests and flatten ClientArgs, forget the socket and document as such. Cheers, As a POE::Component::Server::TCP user (useful to quickly write tests for client components), I prefer option C as I complained about 2 months ago : http://www.mail-archive.com/poe@perl.org/msg04260.html Olivier Mengué.
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
Euh, well, I was meaning option A. But either A or C is good for me. Option B is too awkward as a bad API would stay forever and would bite any new Server::TCP user. Le 21 juillet 2009 12:37, Olivier Mengué a écrit : > > > 2009/7/16 Chris 'BinGOs' Williams > >> Anyways, I see three options: >> >> a). Make the functionality match the documentation; >> >> b). Make the documentation match the functionality; >> >> c). Do what dngor suggests and flatten ClientArgs, forget the socket >>and document as such. >> >> Cheers, >> > > As a POE::Component::Server::TCP user (useful to quickly write tests for > client components), I prefer option C as I complained about 2 months ago : > http://www.mail-archive.com/poe@perl.org/msg04260.html > > Olivier Mengué. >
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
2009/7/16 Chris 'BinGOs' Williams > Anyways, I see three options: > > a). Make the functionality match the documentation; > > b). Make the documentation match the functionality; > > c). Do what dngor suggests and flatten ClientArgs, forget the socket >and document as such. > > Cheers, > As a POE::Component::Server::TCP user (useful to quickly write tests for client components), I prefer option C as I complained about 2 months ago : http://www.mail-archive.com/poe@perl.org/msg04260.html Olivier Mengué.
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Jul 16, 2009, at 07:06, Chris 'BinGOs' Williams wrote: First off, thanks for all the patches, I've applied them all apart from this one. Yes, thank you for the patches, Mark. You're improving POE. And thanks, Chris, for reviewing and applying them. You're accelerating the improvement process. Anyways, I see three options: a). Make the functionality match the documentation; b). Make the documentation match the functionality; c). Do what dngor suggests and flatten ClientArgs, forget the socket and document as such. Option C breaks both the implementation and documentation while realigning the code with its original intent. I think it's good to improve the design a bit, even if it breaks code. The only code that will break is essentially using an undocumented feature anyway. While it's regrettable that the code and/or documentation hasn't been fixed until now, the hypothetical people who ran into the problem didn't report it. I'll accept blame if the documentation was actually correct before the recent rewrite. In that case, I'd have to go with option B. I'd rather have option C, so I might consider deprecating the B-havior in the future. -- Rocco Caputo - rcap...@pobox.com
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
Most people want access to the raw socket to condition it before it's
passed to POE::Wheel::ReadWrite. ClientConnected is a bit late for
this. As Chris Williams mentioned, by the time ClientConnected is
called, the socket has been conditioned and $_[HEAP]{client} is ready
for action.
The component could provide a ClientConnecting callback that triggers
before ClientConnected and allows the server to manipulate the
socket. ClientConnecting would perhaps return the socket if the
connection should succeed, or return undef to disconnect right away.
$_[HEAP]{client} would be undefined since the POE::Wheel::ReadWrite
hasn't been created yet.
This was considered before, but I think it was rejected for some
reason. I think it was discussed in IRC, and the rationale was lost.
Patches are welcome, but please also include tests and documentation.
If the reason for discounting a ClientConnecting callback still
applies, it might come to light during testing.
Unfortunately it seems that rejecting the connection from
ClientConnecting would still incur the overhead of a POE::Session
instantiation and destruction. ClientConnecting must be called from
the context of the per-connection session.
Applications with more specific requirements should take advantage of
POE::Wheel::SocketFactory and POE::Wheel::ReadWrite directly. These
wheels provide more control and performance, with the trade-off being
incrementally more application code.
Sorry for being all formal. I still have my Work Head on.
--
Rocco Caputo - rcap...@pobox.com
On Jul 16, 2009, at 07:29, Nick Williams wrote:
Regarding the passing of the socket into clientconnected - I've
wished for
this in the past, in order to be able to modify TCP options on the
socket,
which didn't seem possible in the past... It would be nice if that was
possible to get at this (either via some available API which I just
haven't
noticed, or if these patches provide it at clientconnected time).
Cheers,
Nick.
On Thu, Jul 16, 2009 at 12:06 PM, Chris 'BinGOs' Williams <
ch...@bingosnet.co.uk> wrote:
On Wed, Jul 15, 2009 at 06:59:12PM -0800, Michael Fowler wrote:
On Wed, Jul 15, 2009 at 04:08:48AM -0400, Rocco Caputo wrote:
For starters, $_[ARG0] isn't guaranteed to contain anything in
particular.
Well, it's guaranteed to contain whatever was passed as the 'args'
argument to the POE::Session constructor, right? When the session
is
constructed, 'args' is [ $socket, $client_args ]. That 'args' value
needs to remain, because the POE::Wheel::ReadWrite constructor
needs it.
Having ClientConnected then mirror _start in terms of arguments
seems
reasonable.
I think you're almost right. The correct behavior would be for
ClientArgs to align with @_[ARG0..$#_]. This still breaks current
code,
but it's cleaner.
Well, I don't mind breaking old code, but I think ARG0 being the
socket
should be preserved. Reaching into $heap->{client}-
>get_input_handle
seems awkward. Then again, perhaps it's breaking an abstraction
to have
direct access to the socket outside of POE::Wheel::ReadWrite.
The code I have that actually needs it would benefit from the
clarity of
having the socket passed as ARG0. Whether or not this breaks some
possible future change in something...
--
Michael Fowler
www.shoebox.net
First off, thanks for all the patches, I've applied them all apart
from this one.
And the main reason for that is deciding what is the best course of
action.
( I have no vested interest in POE::Component::Server::TCP myself
so can have a non-partisan view ).
There is a difference between the documented behaviour and the actual
behaviour.
I, personally, can't see the need to pass the socket object to the
ClientConnected handler, and without any evidence whatsoever to
substantiate my claim >:) I'd suggest that no one actually uses
the socket.
ClientConnected would be used to send any initial text to the client
which one would usually use $_[HEAP]{client} for.
Similarly, one might want to find out the TCP connection details,
which are again in the heap $_[HEAP]{remote_ip}, etc.
My pennyworth.
Anyways, I see three options:
a). Make the functionality match the documentation;
b). Make the documentation match the functionality;
c). Do what dngor suggests and flatten ClientArgs, forget the socket
and document as such.
Cheers,
--
Chris Williams
aka BinGOs
PGP ID 0x4658671F
http://www.gumbynet.org.uk
==
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
Chris 'BinGOs' Williams wrote:
On Thu, Jul 16, 2009 at 12:59:09PM +0100, Ed W wrote:
Nick Williams wrote:
Regarding the passing of the socket into clientconnected - I've wished for
this in the past, in order to be able to modify TCP options on the socket,
which didn't seem possible in the past... It would be nice if that was
possible to get at this (either via some available API which I just haven't
noticed, or if these patches provide it at clientconnected time).
Indeed - I am toying with using POE for a rewrite of a fairly
sophisticated network proxy application over a very low bandwidth
connection and one of my initial questions would have been how to get
hold of the socket in order to set a bunch of badly documented OS
specific options on it (buffer sizes, etc)
Getting hold of the socket object at listen and connect time is very
valuable
You have access to the ReadWrite wheel in $_[HEAP]{client}, so you
have access to the socket via $_[HEAP]{client}->get_input_handle
Since this is all theoretical to me at this stage, then this sounds just
fine to me!
Ed W
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Thu, Jul 16, 2009 at 12:59:09PM +0100, Ed W wrote:
> Nick Williams wrote:
> >Regarding the passing of the socket into clientconnected - I've wished for
> >this in the past, in order to be able to modify TCP options on the socket,
> >which didn't seem possible in the past... It would be nice if that was
> >possible to get at this (either via some available API which I just haven't
> >noticed, or if these patches provide it at clientconnected time).
> >
>
>
> Indeed - I am toying with using POE for a rewrite of a fairly
> sophisticated network proxy application over a very low bandwidth
> connection and one of my initial questions would have been how to get
> hold of the socket in order to set a bunch of badly documented OS
> specific options on it (buffer sizes, etc)
>
> Getting hold of the socket object at listen and connect time is very
> valuable
You have access to the ReadWrite wheel in $_[HEAP]{client}, so you
have access to the socket via $_[HEAP]{client}->get_input_handle
Cheers,
--
Chris Williams
aka BinGOs
PGP ID 0x4658671F
http://www.gumbynet.org.uk
==
pgpr7qifjbX5M.pgp
Description: PGP signature
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
Nick Williams wrote: Regarding the passing of the socket into clientconnected - I've wished for this in the past, in order to be able to modify TCP options on the socket, which didn't seem possible in the past... It would be nice if that was possible to get at this (either via some available API which I just haven't noticed, or if these patches provide it at clientconnected time). Indeed - I am toying with using POE for a rewrite of a fairly sophisticated network proxy application over a very low bandwidth connection and one of my initial questions would have been how to get hold of the socket in order to set a bunch of badly documented OS specific options on it (buffer sizes, etc) Getting hold of the socket object at listen and connect time is very valuable Cheers Ed W
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
Regarding the passing of the socket into clientconnected - I've wished for
this in the past, in order to be able to modify TCP options on the socket,
which didn't seem possible in the past... It would be nice if that was
possible to get at this (either via some available API which I just haven't
noticed, or if these patches provide it at clientconnected time).
Cheers,
Nick.
On Thu, Jul 16, 2009 at 12:06 PM, Chris 'BinGOs' Williams <
ch...@bingosnet.co.uk> wrote:
> On Wed, Jul 15, 2009 at 06:59:12PM -0800, Michael Fowler wrote:
> > On Wed, Jul 15, 2009 at 04:08:48AM -0400, Rocco Caputo wrote:
> > > For starters, $_[ARG0] isn't guaranteed to contain anything in
> particular.
> >
> > Well, it's guaranteed to contain whatever was passed as the 'args'
> > argument to the POE::Session constructor, right? When the session is
> > constructed, 'args' is [ $socket, $client_args ]. That 'args' value
> > needs to remain, because the POE::Wheel::ReadWrite constructor needs it.
> > Having ClientConnected then mirror _start in terms of arguments seems
> > reasonable.
> >
> >
> > > I think you're almost right. The correct behavior would be for
> > > ClientArgs to align with @_[ARG0..$#_]. This still breaks current
> code,
> > > but it's cleaner.
> >
> > Well, I don't mind breaking old code, but I think ARG0 being the socket
> > should be preserved. Reaching into $heap->{client}->get_input_handle
> > seems awkward. Then again, perhaps it's breaking an abstraction to have
> > direct access to the socket outside of POE::Wheel::ReadWrite.
> >
> > The code I have that actually needs it would benefit from the clarity of
> > having the socket passed as ARG0. Whether or not this breaks some
> > possible future change in something...
> >
> > --
> > Michael Fowler
> > www.shoebox.net
>
> First off, thanks for all the patches, I've applied them all apart
> from this one.
>
> And the main reason for that is deciding what is the best course of
> action.
>
> ( I have no vested interest in POE::Component::Server::TCP myself
> so can have a non-partisan view ).
>
> There is a difference between the documented behaviour and the actual
> behaviour.
>
> I, personally, can't see the need to pass the socket object to the
> ClientConnected handler, and without any evidence whatsoever to
> substantiate my claim >:) I'd suggest that no one actually uses
> the socket.
>
> ClientConnected would be used to send any initial text to the client
> which one would usually use $_[HEAP]{client} for.
>
> Similarly, one might want to find out the TCP connection details,
> which are again in the heap $_[HEAP]{remote_ip}, etc.
>
> My pennyworth.
>
> Anyways, I see three options:
>
> a). Make the functionality match the documentation;
>
> b). Make the documentation match the functionality;
>
> c). Do what dngor suggests and flatten ClientArgs, forget the socket
>and document as such.
>
> Cheers,
>
> --
> Chris Williams
> aka BinGOs
> PGP ID 0x4658671F
> http://www.gumbynet.org.uk
> ==
>
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Wed, Jul 15, 2009 at 06:59:12PM -0800, Michael Fowler wrote:
> On Wed, Jul 15, 2009 at 04:08:48AM -0400, Rocco Caputo wrote:
> > For starters, $_[ARG0] isn't guaranteed to contain anything in particular.
>
> Well, it's guaranteed to contain whatever was passed as the 'args'
> argument to the POE::Session constructor, right? When the session is
> constructed, 'args' is [ $socket, $client_args ]. That 'args' value
> needs to remain, because the POE::Wheel::ReadWrite constructor needs it.
> Having ClientConnected then mirror _start in terms of arguments seems
> reasonable.
>
>
> > I think you're almost right. The correct behavior would be for
> > ClientArgs to align with @_[ARG0..$#_]. This still breaks current code,
> > but it's cleaner.
>
> Well, I don't mind breaking old code, but I think ARG0 being the socket
> should be preserved. Reaching into $heap->{client}->get_input_handle
> seems awkward. Then again, perhaps it's breaking an abstraction to have
> direct access to the socket outside of POE::Wheel::ReadWrite.
>
> The code I have that actually needs it would benefit from the clarity of
> having the socket passed as ARG0. Whether or not this breaks some
> possible future change in something...
>
> --
> Michael Fowler
> www.shoebox.net
First off, thanks for all the patches, I've applied them all apart
from this one.
And the main reason for that is deciding what is the best course of
action.
( I have no vested interest in POE::Component::Server::TCP myself
so can have a non-partisan view ).
There is a difference between the documented behaviour and the actual
behaviour.
I, personally, can't see the need to pass the socket object to the
ClientConnected handler, and without any evidence whatsoever to
substantiate my claim >:) I'd suggest that no one actually uses
the socket.
ClientConnected would be used to send any initial text to the client
which one would usually use $_[HEAP]{client} for.
Similarly, one might want to find out the TCP connection details,
which are again in the heap $_[HEAP]{remote_ip}, etc.
My pennyworth.
Anyways, I see three options:
a). Make the functionality match the documentation;
b). Make the documentation match the functionality;
c). Do what dngor suggests and flatten ClientArgs, forget the socket
and document as such.
Cheers,
--
Chris Williams
aka BinGOs
PGP ID 0x4658671F
http://www.gumbynet.org.uk
==
pgpvxPjGYdYp5.pgp
Description: PGP signature
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Wed, Jul 15, 2009 at 11:18:03PM -0400, sungo wrote: > Perhaps I'm misreading... What I'm hearing is "you should change this > for me, regardless of whether it breaks other people's code". Is that an > accurate summary? No. That was in response to expanding ClientArgs into ARG0..$#_. I'm suggesting if compatibility is going to be broken, it should be broken to fit the documentation, and to be useful. -- Michael Fowler www.shoebox.net
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On (07/15 18:59), Michael Fowler wrote: > The code I have that actually needs it would benefit from the clarity of > having the socket passed as ARG0. Whether or not this breaks some > possible future change in something... Perhaps I'm misreading... What I'm hearing is "you should change this for me, regardless of whether it breaks other people's code". Is that an accurate summary? -- sungo http://sungo.us signature.asc Description: Digital signature
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Wed, Jul 15, 2009 at 04:08:48AM -0400, Rocco Caputo wrote:
> For starters, $_[ARG0] isn't guaranteed to contain anything in particular.
Well, it's guaranteed to contain whatever was passed as the 'args'
argument to the POE::Session constructor, right? When the session is
constructed, 'args' is [ $socket, $client_args ]. That 'args' value
needs to remain, because the POE::Wheel::ReadWrite constructor needs it.
Having ClientConnected then mirror _start in terms of arguments seems
reasonable.
> I think you're almost right. The correct behavior would be for
> ClientArgs to align with @_[ARG0..$#_]. This still breaks current code,
> but it's cleaner.
Well, I don't mind breaking old code, but I think ARG0 being the socket
should be preserved. Reaching into $heap->{client}->get_input_handle
seems awkward. Then again, perhaps it's breaking an abstraction to have
direct access to the socket outside of POE::Wheel::ReadWrite.
The code I have that actually needs it would benefit from the clarity of
having the socket passed as ARG0. Whether or not this breaks some
possible future change in something...
--
Michael Fowler
www.shoebox.net
Re: POE::Component::Server::TCP bug fixes, possibly incompatible
On Jul 14, 2009, at 02:16, Michael Fowler wrote: http://rt.cpan.org/Ticket/Display.html?id=47855 47855 requires a bit of discussion. The problem is that the ClientConnected callback does not actually receive a socket in ARG0. ARG0 has been spliced off in the POE::Wheel::ReadWrite constructor, so ARG0 is actually the ClientArgs parameter. This change was implemented nearly 4 years ago (to fix a leaking socket problem), so anything that actually uses ARG0 will break if the code is changed to match the documentation. How should the code be changed? Should it be made conservatively, so that any code relying on ARG0 being ClientArgs continues to work? Should it be fixed to match the documentation? The documentation is wrong. I misread the code and thought that ClientConnected was part of the POE::Wheel::SocketFactory SuccessEvent handler. It's not. It's really part of the client-handling session's _start handler, which has different semantics. For starters, $_[ARG0] isn't guaranteed to contain anything in particular. One advantage of out and out breaking old code is that ClientArgs could actually be flattened into ARG1..$#_. I think you're almost right. The correct behavior would be for ClientArgs to align with @_[ARG0..$#_]. This still breaks current code, but it's cleaner. -- Rocco Caputo - rcap...@pobox.com
POE::Component::Server::TCP bug fixes, possibly incompatible
Greetings, During the course of writing a tutorial I've been noticing discrepancies between POE::Component::Server::TCP's documentation and code. I have filed some initial tickets: http://rt.cpan.org/Ticket/Display.html?id=47855 http://rt.cpan.org/Ticket/Display.html?id=47854 http://rt.cpan.org/Ticket/Display.html?id=47853 http://rt.cpan.org/Ticket/Display.html?id=47852 Most are pretty straightforward. However, 47855 requires a bit of discussion. The problem is that the ClientConnected callback does not actually receive a socket in ARG0. ARG0 has been spliced off in the POE::Wheel::ReadWrite constructor, so ARG0 is actually the ClientArgs parameter. This change was implemented nearly 4 years ago (to fix a leaking socket problem), so anything that actually uses ARG0 will break if the code is changed to match the documentation. How should the code be changed? Should it be made conservatively, so that any code relying on ARG0 being ClientArgs continues to work? Should it be fixed to match the documentation? One advantage of out and out breaking old code is that ClientArgs could actually be flattened into ARG1..$#_. Right now it's a little awkward and confusing that ClientArgs is required to be an arrayref, yet nothing in POE::Component::Server::TCP is actually using it as one. -- Michael Fowler www.shoebox.net
