143172 - set low connect timeout for LDAP XPCOM SDK by default

Status: VERIFIED FIXED

(Directory :: LDAP XPCOM SDK, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This is a very simple fix pointed out by mcs that makes bug 79509 much less
painful.  Users will perceive it as a hiccup or stutter by the browser suite,
not a hang that forces them to kill the entire thing.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

In bug 79509, mcs said:

> Also, note that you should be able to reduce the connect timeout when you are
> NOT using the LDAP_OPT_ASYNC_CONNECT option by making a call like this:
>
> int ms = 10000; /* 10 seconds, in milliseconds */
> if ( ldap_set_option( 0, LDAP_X_OPT_CONNECT_TIMEOUT, *ms ) != 0 ) {
>      /* error */
> }

[...]

> IMHO a timeout of 10 seconds seems more than long enough for autocomplete (of 
> course it should be configurable).

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: set an upper limit of 5 seconds on all I/O operations

I decided to use the prldap mechanism instead, as this will save us from
infinite hangs on all operations; I also chose a slightly lower timeout of 5
seconds.  Yeah, this should be a preference, and I'll file a bug for that, but
I'd like to get this into 1.0, so I'm trying to keep the patch small.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

As far as the old XXX comment, I think that was just because I was setting the
timeout to 0, rather than to something large enough to succeed most of the time.

Reviews welcomed.  :-)

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 82881 [details] [diff] [review]
set an upper limit of 5 seconds on all I/O operations

sr=sspitzer

Comment 5

[scottputterman]

Dan, how confident are you about what you wrote in comment #3 about your code
comment not being something we should be worried about?

Let's get a r= for this.  I'll bring this to adt.

Comment 6

[Mark Smith [:mcs]]

Comment on attachment 82881 [details] [diff] [review]
set an upper limit of 5 seconds on all I/O operations

r=mcs, but I am a little worried that 5 seconds will be too short for the
initial TCP connect timeout (it really depends on the network between your
machine and the LDAP server). But the default TCP connect timeout is 1-2
minutes or so on most platforms. I still vote for 10 seconds, but can live with
5.

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 82881 [details] [diff] [review]
set an upper limit of 5 seconds on all I/O operations

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 8

[scottputterman]

If it would be safer to do 10 seconds then let's do that.  It's still way better
than a couple of minutes.

Could we get this in a test build for QA?  If we need to respin and it's been
verified, this would be worth considering for beta.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

putterman: I'd say I'm about 75% confident of comment number 3.  I'll generate a
new patch for switching to 10 seconds and checkin on the trunk.  So trunk builds
for tommorrow should have this patch, and QA can test against those.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: set an upper limit of 10 seconds on all I/O operations

Bump upper limit to 10 seconds.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 82980 [details] [diff] [review]
set an upper limit of 10 seconds on all I/O operations

Carrying forward review, super-review, approval.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

QA: a good way to forcibly provoke the hang for testing purposes is to set up a
server pointing at IP address 1.1.1.1 port 389, and try an LDAP operation
against that.

Comment 13

[(not reading, please use [e-mail] instead)]

I just tried out dmose's patch on my trunk, mozilla, win2k build.

It worked like a charm.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in on the trunk.

Comment 15

[scottputterman]

adding adt1.0.0+ for checkin to mozilla1.0 branch.

Comment 16

[yulian chang]

20020510 trunk build on MacOS 9.1 and Linux

Works fine.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

This won't make the Netscape beta train, so resolving as fixed.

Comment 18

[yulian chang]

Verified with 20020605 branch build on Win2K and MacOS 9.2.