Explicit timeout for connecting OpenID provider

[GoogleCodeExporter]

Recently one of the OpenID providers was very slow, and was way beyond the
timeout I'd like to have for my application. How can I tell the library the
timeout I want?

What steps will reproduce the problem?
1. Implement a hanging JSP, e.g. <% Thread.sleep(100000); %>
2. Try to login using this JSP as OpenID URL
3. Hmm... how to control how long the discovery waits?

What is the expected output? What do you see instead?

The discover() method should take (directly or indirectly) an expected
timeout. If OpenID provider is not responding within this timeout, a
special exception should be thrown (so I can tell user it's a timeout
problem, not wrong credentials).

What version of the product are you using? On what operating system?

1.0.6. Windows and Linux.

Original issue reported on code.google.com by Vladimir...@gmail.com on 13 Feb 2009 at 12:36

[GoogleCodeExporter]

Hi Vlad,

Since the default is using HttpUrlConnection, just configure the system 
property:
-Dsun.net.client.defaultConnectTimeout=8000

That's in milliseconds.

Cheers,
David

Original comment by david.yu...@gmail.com on 13 Feb 2009 at 10:15

[GoogleCodeExporter]

That's not a good idea. That's JVM-wide setting, and it affects other 
applications
deployed on the server (yes, I have more than one).

The setting should be programmatical, i.e. either by passing a property to
RelayingParty (preferrable) or even as a parameter to discover(). 

I'm now considering to increase timeout for one particular OpenID provider 
(which is
in Russia and quite slow to respond) while keeping others under 30s. You see, 
the
JVM-wide setting is not fitting well here.

If you like, I can implement it in a branch and let you merge (or reject, 
whatever)
the result.

Original comment by Vladimir...@gmail.com on 13 Feb 2009 at 1:52

[GoogleCodeExporter]

I'll definitely make it configurable for the upcoming release.
For now, you'll have to subclass SimpleHttpConnector and customize the returned
response with the interval.

You are welcome to submit a patch.

Cheers

Original comment by david.yu...@gmail.com on 13 Feb 2009 at 4:51

[GoogleCodeExporter]

Ok, I've done it. The change required is very simple:

1. add setTimeout(int timeout) to HttpConnector interface
2. add setTimeout implementation to SimpleHttpConnector class (save value into
instance variable)
3. make SimpleHttpConnector.send() a non-static class
4. in SimpleHttpConnector.send add 

        connection.setConnectTimeout((int) timeout);
        connection.setReadTimeout((int) timeout);

before calling connect()

5. in servlet, before invoking discover(), do

HttpConnector httpConnector = rp.getOpenIdContext().getHttpConnector();
httpConnector.setTimeout(timeout);

based on timeout specified by configuration for identity.

The solution works for me, but I don't like it much. Ideally, the timeout 
should be
set for id _after_ resolving but before discovery. Otherwise I have to repeat 
the
logic in Resolver class. That would require a method Listener.beforeDiscovery() 
which
is missed currently, and that method shall have access to context.

Original comment by Vladimir...@gmail.com on 15 Feb 2009 at 4:47

[GoogleCodeExporter]

One more thingy to fix: timeout is twice as high as specified. That's because of
YadisDiscovery tried first. If we have got timeout there, there is no point to 
try
another way, the host is too slow. So:

YadisDiscovery.discoverXRDS -- remove catch(Exception){ user = null }

This catch is useless anyway: user would be not null only if there is no 
exception
happend.

Original comment by Vladimir...@gmail.com on 16 Feb 2009 at 3:08

[GoogleCodeExporter]

ouch, one more :)

YadisDiscovery.tryDiscovery:

Response response = context.getHttpConnector().doHEAD(identifier.getUrl(), 
null);
response.getInputStream(); // to trigger SocketTimeoutException if happend
String location = response.getHeader(X_XRDS_LOCATION);
if(location==null)
...

If inputStream is not obtained here, the SocketTimeoutException is not thrown
(despite the wait is cancelled after set time), so we cannot catch
SocketTimeoutException in DefaultDiscovery and abandon further attempts.

Original comment by Vladimir...@gmail.com on 16 Feb 2009 at 4:13

[GoogleCodeExporter]

Thanks for your feedback.

The timeout can now be set via:
-Dshc.connect_timeout=5000 (default: 10000)
-Dshc.follow_redirects=false (default: true)

or ...
SimpleHttpConnector.setConnectTimeout(5000);
SimpleHttpConnector.setFollowRedirects(false);

I've removed the catch block on DefaultDiscovery so that the connector can 
throw the 
timeout exception.

Your application can handle this exception if you do:
catch(InterruptedIOException e) or
catch(SocketTimeoutException e)

Fix is in trunk.

Cheers

Original comment by dyuproj...@gmail.com on 16 Feb 2009 at 9:32

• Changed state: Fixed