[Redis] Redis client: Timeouts !

[Damien Krotkine]

 Le 27 déc. 2013 à 00:30, Steffen Mueller <smueller at cpan.org> a écrit :

> Hi dams,
> 
> On 12/26/2013 11:28 PM, Damien Krotkine wrote:
>> I've just pushed new code in the git master branch. It contains timeouts
>> related features. Now you can creates a Redis client with connection,
>> read and write timeouts:
>> 
>> my $redis = Redis->new(
>>     cnx_timeout => 60,      # connection timeout (in seconds)
>>     read_timeout => 0,5,    # read timeout (in seconds)
>>     write_timeout => 1.2,    # write timeout (in seconds)
>> );
>> 
>> I haven't yet released it on CPAN, I'd like to do a developer release
>> first (but I'm not sure how to do that with dzil). Anyway please feel
>> free to test it and let me know of any issues. The code uses 2
>> strategies, on mac and linux it uses setsockopt to set read / write
>> timeouts, on other OS it uses 'select'. I plan to extend the setsockopt
>> strategies to more OS.
> 
> Reading the change and having another look at IO::Socket::Timeout, I have some concerns about the implementation. IO::Socket::Timeout implements a function "new::with::timeout" (and socketpair::with::timeout) which is then to be called as a fully qualified method on a compatible class.
> 
> That's a real hack. For those reading along that might not immediately see what's going on: This completely bypasses method dispatch and subverts Perl's object model. new::with::timeout is the "timeout" function in the "new::with" namespace. $class->new::with::timeout is exactly the same as calling a fully qualified function with a string argument "new::with::timeout($class)". There's not method dispatch happening and another namespace is somewhat subtly polluted.
> 
> Surely, there's a less invasive and less likely-to-make-somebody-lose-days-debugging way of implementing this?

This is a valid concern :) I spent a lot of time thinking about the interface of the module, and discussed it with few people prior to implementation. 

The initial problem is that I wanted to provide a module that works with all kind of socket, be it Unix, INET, or Socket built from scratch, etc. Ideally, I wanted to be as independent possible, as long as the class provides a new() method that returns a reference on a typeglob. 

So in a sense, what I want to implement conceptually is a role.

IO::Socket family of classes uses standard objects, so it's difficult to provide a role that works with it. For example, IO::Socket::INET has IO::Socket as base class, hardcoded.   

Basically I was left with:

- pollute the Socket class ( monkey-patching )
- or pollute each of the children classes ( more monkey-patching and not future proof )
- provide an ugly API like IO::Socket::Timeout->create_with_timeout( class => 'IO::Socket::Unix', ...)
- use the same approach than Import::Into

I think it's leont who suggested the import::into way. Yes, it pollutes the 'new::with' namespace ( but I doubt this will be an issue, ever) as Import::into pollutes its namespace. Yes, some people may think that the code added a new method in IO::Socket. But to me, it's not a hack, or it's at least less hackish than monkey patching. But maybe I'm missing other issues of this implementation ?

Ithere is a better way, I'd be happy to change the implementation.