Skip Menu |
 

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

Report information
The Basics
Id: 95983
Status: resolved
Priority: 0/
Queue: IO-Socket-IP

People
Owner: Nobody in particular
Requestors: sullr [...] cpan.org
Cc: CARNIL [...] cpan.org
gregoa [...] cpan.org
AdminCc:

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



Subject: IO::Socket::IP should not call $self->connect when retrying connect
Download (untitled) / with headers
text/plain 900b
Hi, currently IO::Socket::IP implements multi-homing by calling $self->connect for all possible combinations of source and destination until it succeeds. But, if IO::Socket::IP is used as a base class for another module, like done with IO::Socket::SSL, this will not result in retrying until it gets an successful connection to an IP, but until the connect of the derived class returns success. In the case of IO::Socket::SSL successful connect will not only mean connection to the IP address, but also SSL handshake, verification of the certificate etc. And in my opinion it should not be the behavior of the IP layer to retry a connection if the SSL layer failed because of certificate validation errors. So I would suggest, that any $self->connect should be changed to $self->IO::Socket::IP::connect, so that the class deals only about successful connection on the IP level. Regards, Steffen
Download (untitled) / with headers
text/plain 642b
On Mon May 26 15:49:16 2014, SULLR wrote: Show quoted text
> So I would suggest, that any $self->connect should be changed to > $self->IO::Socket::IP::connect, so that the class deals only about > successful connection on the IP level.
Yeees. I think that sounds a reasonable idea. As an aside: I think it's a shame in general there isn't a neater way to notate that kind of thing. It would be nice if you could subclass an object and provide a new definition for an existing method, but which only applies to "outsiders", and existing calls in base classes still see their base definition. Anyway. Patch attached; will be in next release. -- Paul Evans
Subject: rt95983.patch
Download rt95983.patch
text/x-diff 758b
=== modified file 'lib/IO/Socket/IP.pm' --- lib/IO/Socket/IP.pm 2014-02-24 16:08:13 +0000 +++ lib/IO/Socket/IP.pm 2014-05-31 17:23:48 +0000 @@ -601,7 +601,7 @@ } if( defined( my $addr = $info->{peeraddr} ) ) { - if( $self->connect( $addr ) ) { + if( $self->IO::Socket::IP::connect( $addr ) ) { $! = 0; return 1; } @@ -651,7 +651,7 @@ # (still in progress). This even works on MSWin32. my $addr = ${*$self}{io_socket_ip_infos}[${*$self}{io_socket_ip_idx}]{peeraddr}; - if( $self->connect( $addr ) or $! == EISCONN ) { + if( $self->IO::Socket::IP::connect( $addr ) or $! == EISCONN ) { delete ${*$self}{io_socket_ip_connect_in_progress}; $! = 0; return 1;
Download (untitled) / with headers
text/plain 234b
I may have to revert this one because it's causing bad knock-on effects with IO::Socket::SSL: https://rt.cpan.org/Ticket/Display.html?id=97050 Basically: the very thing it was supposed to fix, it has broken. Meh. -- Paul Evans
Download (untitled) / with headers
text/plain 1.4k
Am Di 08. Jul 2014, 06:35:58, PEVANS schrieb: Show quoted text
> I may have to revert this one because it's causing bad knock-on > effects with IO::Socket::SSL: > > https://rt.cpan.org/Ticket/Display.html?id=97050 > > Basically: the very thing it was supposed to fix, it has broken. Meh.
Yes, unfortunately it wasn't as easy as I thought because the calling scheme inside IO::Socket::* (i.e. new -> configure -> connect ) isn't that simple if you have a class hierarchy and also try to implement multi-homing :( But I think I have a working patch (included, against 0.30). The basic idea of the patch is that one has to distinguish between an error at the transport layer which can be solved with IP based multi-homing and an error at the application layer. One could expect the system error to be reflected inside $!, while an application error will probably not set $! (e.g. IO::Socket::SSL sets an $SSL_ERROR variable). So if connect fails, but $! is not set, one can assume error at the application layer and stop trying to fix it with IP based multi-homing. The other difference in the patch is to change $self->IO::Socket::IP::connect($addr) to CORE::connect($self,$addr), because if you have a look at the connect function it simple calls CORE::connect if an $addr argument is given. It was already right to not use $self->connect in this place, it was only a problem if called from inside the new - configure - connect chain. With this patch the tests inside IO::Socket::IP pass and also the tests of IO::Socket::SSL. Regards, Steffen
Subject: IP.pm.patch
Download IP.pm.patch
text/x-diff 1.1k
diff --git a/lib/IO/Socket/IP.pm b/lib/IO/Socket/IP.pm index 1911145..16eb7c8 100644 --- a/lib/IO/Socket/IP.pm +++ b/lib/IO/Socket/IP.pm @@ -601,7 +601,7 @@ sub setup } if( defined( my $addr = $info->{peeraddr} ) ) { - if( $self->IO::Socket::IP::connect( $addr ) ) { + if( $self->connect( $addr ) ) { $! = 0; return 1; } @@ -611,6 +611,13 @@ sub setup return 0; } + # If connect failed but we have no system error there must be an error + # at the application layer, like a bad certificate with + # IO::Socket::SSL. + # In this case don't continue IP based multi-homing because the problem + # cannot be solved at the IP layer. + return 0 if ! $!; + ${*$self}{io_socket_ip_errors}[0] = $!; next; } @@ -651,7 +658,7 @@ sub connect # (still in progress). This even works on MSWin32. my $addr = ${*$self}{io_socket_ip_infos}[${*$self}{io_socket_ip_idx}]{peeraddr}; - if( $self->IO::Socket::IP::connect( $addr ) or $! == EISCONN ) { + if( CORE::connect( $self, $addr ) or $! == EISCONN ) { delete ${*$self}{io_socket_ip_connect_in_progress}; $! = 0; return 1;
Fixed by effectively reverting the original change :/ -- Paul Evans


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.