On Oct 6, 2011, at 09:43 , Jared Johnson wrote:
>> $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,
can_read will only come back true if there is data to read. If there is a
timeout
then it will return zero and the for loop will not be entered.
->can_read(0) does not block, it polls only.
> 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.
That is only called if there is a problem reading a complete packet, but there
has to be some data on the socket to get there.
> 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
any timeout check should probably be done inside sync, not process. process is
only supposed to be called when there is data on the socket to process.
in fact what you highlight is that currently the client can go into a complete
cpu spin while waiting for data from the server
Graham.
>
>> 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
>
