> $sock->timeout is intended to be a connect timeout. Why should read
> timeout be the same.
well, it seemed like a good compromise short of providing a new API, which
I didn't have any bright ideas about. If you thought it best to provide
such an API, I wouldn't object to using it :) Also, I guess I incorrectly
interpreted Net::LDAP's passing the timeout from new() to the underlying
socket and doing nothing else, as intending to do more than just implement
a connect timeout; e.g. at least one subsequent operation,
$ldap->start_tls(), inherits the same timeout. I assumed there were other
such operations affected, maybe there are not.
>>> my $sel = IO::Select->new($sock);
>>> my $ready;
>>>
>>> - for( $ready = 1 ; $ready ; $ready = $sel->can_read(0)) {
>>> + for( $ready = 1 ; $ready ; $ready = $sel->can_read( $timeout )) {
>
> This would not be sufficient to stop the client hanging. ->process is
> called from within ->sync which will keep calling process until it gets
> the message response it is looking for.
The way I read that loop, as long as we can get can_read() to come back,
if the socket is not ready then it will behave the same as if the server
had dropped the connection - asn_read() will fail, causing process() to
return 'return _drop_conn($ldap, LDAP_OPERATIONS_ERROR, "Communications
Error")', which seems like a fairly appropriate error. If I'm reading it
wrong, then the effects of this change on our hanging-forever bug are odd
-- I can't imagine calling can_read($timeout) over and over again waiting
for it to come back would behave any differently than calling can_read(0),
and yet we did observe very different behavior to the effect of fixing our
bug. Granted it could be that I managed to break things differently
rather than fix them :P
> Also if this message being waited on was the result of a call to, say,
> ->search. The callback for the search should be called with a client side
> error
>From my reading above I would think search() would return 'Communications
Error' as well. I was not thorough enough in my testing to say this for
sure though. I'd be willing to go back and test, if you felt this first
patch was conceptually a good idea at all. If you'd rather see this done
with larger design and/or API changes, I don't know that I'd be your man.
>>> + if ( $arg->{timeout} and IO::Socket->can('SO_SNDTIMEO') ) {
>>> + my $timeval = pack 'L!L!', $ldap_opts->{timeout}, 0;
>>> + eval {
>>> + $obj->{net_ldap_socket}->sockopt( SO_SNDTIMEO, $timeval ) or die
>>> $!;
>>> + $obj->{net_ldap_socket}->sockopt( SO_RCVTIMEO, $timeval ) or die
>>> $!;
>>> + };
>
> Using low level socket options is almost never the right thing to be
> doing. using select or poll would be the right solution.
Fair enough... I admit this was a cheap shortcut to deal with the observed
shortcomings of the above patch, that is, it seems to enforce that any
single operation will time out in T seconds rather than T * N seconds
-Jared
