DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign

[EMAIL PROTECTED] changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution||FIXED



--- Additional Comments From [EMAIL PROTECTED]  2004-04-13 21:49 ---
Patch committed.

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-13 17:10 ---
Folks,
Any further remarks? If nobody objects loudly, I'll fix javadocs and commit the
patch within next 4-5 hours. If any problem pops up later, it can always be
addressed with a corrective patch

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-13 02:48 ---
Looks good to me, except for some JavaDocs problems.  The docs for 
ReflectionSocketFactory.createSocket() and the ProtocolSocketFactory.createSocket() 
are out of date.

Mike

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 21:41 ---
Created an attachment (id=11219)
Patch (take 4)

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 21:33 ---
Hi Sam,
First of all, thanks for inspiring me to fix this problem (and donating initial
reflection code)

> doing don't include the step that uses "SocketFactory.getDefault()".  This
> confused me mostly because getDefault was static, and I didn't realize that it
> was expected that each subclassed SocketFactory would implement it's own
> getDefault behaviour.

AFAIK SocketFactory#getDefaut is the only (standard) way to instantiate a socket
factory. Therefore I thought this step required no explanation. Fixed now

> It also seems a bit odd to use the String equality to test for an exception 
of SocketTimeoutException. 

Fair enough. Fixed. Patch coming.

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 18:03 ---
A tiny detail, but a possibly confusing one for people reading the code -- the
comments in ReflectionSocketFactory that explain what the reflection code is
doing don't include the step that uses "SocketFactory.getDefault()".  This
confused me mostly because getDefault was static, and I didn't realize that it
was expected that each subclassed SocketFactory would implement it's own
getDefault behaviour.

It also seems a bit odd to use the String equality to test for an exception of
SocketTimeoutException.  Perhaps now that the classes are being statically
cached, a SocketTimeoutException.class could be created and the catch clause
could use SOCKETTIMEOUTEXCEPTION_CLASS.isInstance(cause).

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 17:49 ---
Fair enough. So, how about this?

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 17:48 ---
Created an attachment (id=11218)
Patch (take 3)

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 14:28 ---
I would suggest caching the references to the method and constructor objects. In
1.3 VM's, that's a 3:1 performance difference. In 1.4 it's still a 2:1
performance improvement. 
http://jguru.com/faq/view.jsp?EID=246569

\jdk1.3.1_06\bin\java -cp . PerformanceTest
10 regular method calls:219 milliseconds.
10 reflective method calls without lookup:281 milliseconds.
10 reflective method calls with lookup:954 milliseconds.

\jdk1.4.2\bin\java -cp . PerformanceTest
10 regular method calls:282 milliseconds.
10 reflective method calls without lookup:328 milliseconds.
10 reflective method calls with lookup:625 milliseconds.

Moh

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 13:47 ---
How about this? Let me know what you think.

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 13:47 ---
Created an attachment (id=11217)
Patch (take 2)

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 12:46 ---
> Completely agree. The trouble is that javax.net.SocketFactory is available as of
> Java 1.4 only. I was thinking about using ProtocolSocketFactory instead but
> initially decided against it, as it results in a sort of logical recursion: a
> helper class takes a class as a parameter for which it acts as a helper class.
> Thinking about it causes stack overflow in my brain requiring a hard reboot with
> a glass of malt whisky. But I'll give it another shot, as it can potentially a
> lot of ugly code in the protocol socket factories

Good point. SocketFactory is a part of JSSE, but it may be annoying for users to 
include JSSE if they're 
not using it.  This may be a lost cause.

> I seriously do not know what code may be moved to a static initializer unless we
> create a dummy socket and try calling connect on it. But to which port? Am I
> missing something? What if we just set a static flag: refection failed, do not
> try it again? 

This is what I was thinking:

private static boolean connectSupported;
private static Constructor insetSocketAddressConstructor;
private static Method connectMethod;

static {
try {
Class addressClass = Class.forName("java.net.InetSocketAddress");
insetSocketAddressConstructor = addressClass.getConstructor(
new Class[] { String.class, Integer.TYPE });
connectMethod = Socket.class.getMethod("connect", 
new Class[] {Class.forName("java.net.SocketAddress"), Integer.TYPE});
connectSupported = true;
} catch (Exception e) {
// 1.4 socket connect method not supported
connectSupported = false;
}
}

Inside createSocket() we could just test 'connectSupported' instead of trying the 
reflection.  We can also 
reuse the Constructor and Method to avoid looking them up again.

Mike

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-12 12:27 ---
> I think we should change ControllerThreadSocketFactory.createSocket() 
> to accept a SocketFactory and all the other params necessary to create 
> the socket.  This way we can hide the SocketTask from external 
> classes.  Though we would end up passing in more params I think it would
> make for a cleaner interface.

Completely agree. The trouble is that javax.net.SocketFactory is available as of
Java 1.4 only. I was thinking about using ProtocolSocketFactory instead but
initially decided against it, as it results in a sort of logical recursion: a
helper class takes a class as a parameter for which it acts as a helper class.
Thinking about it causes stack overflow in my brain requiring a hard reboot with
a glass of malt whisky. But I'll give it another shot, as it can potentially a
lot of ugly code in the protocol socket factories

> We should consider changing ProtocolSocketFactory.createSocket(host, 
> port ... timeout) to accept a HttpConnectionParams instead of a timeout.  
> This will allow for other config params in the future without requiring
> API changes.

Great idea

> I think ReflectionSocketFactory.createSocket() should be changed to take a
> SocketFactory instance instead of a socketFactoryName.  This is not a major
> change, but it seems a little cleaner to me.

Same problem as in point 1

> I suggest we move the bulk of the reflection work from 
> ReflectionSocketFactory.createSocket() to a static initializer.  
> This way we can determine statically whether or not the JVM supports
> Socket.connect() without testing each time.  This may be premature
> optimization, but my feeling is that it may be worth it.

I seriously do not know what code may be moved to a static initializer unless we
create a dummy socket and try calling connect on it. But to which port? Am I
missing something? What if we just set a static flag: refection failed, do not
try it again? 

Oleg

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign

[EMAIL PROTECTED] changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Target Milestone|--- |2.1 Alpha 1

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-11 22:35 ---
Hi Oleg,

I like the changes. This is definitely the way to go.  I have a couple of suggestions.

 - I think we should change ControllerThreadSocketFactory.createSocket() to accept a 
SocketFactory and 
all the other params necessary to create the socket.  This way we can hide the 
SocketTask from external 
classes.  Though we would end up passing in more params I think it would make for a 
cleaner interface.

 - We should consider changing ProtocolSocketFactory.createSocket(host, port ... 
timeout) to accept a 
HttpConnectionParams instead of a timeout.  This will allow for other config params in 
the future 
without requiring API changes.

 - I think ReflectionSocketFactory.createSocket() should be changed to take a 
SocketFactory instance 
instead of a socketFactoryName.  This is not a major change, but it seems a little 
cleaner to me.

 - I suggest we move the bulk of the reflection work from 
ReflectionSocketFactory.createSocket() to a 
static initializer.  This way we can determine statically whether or not the JVM 
supports Socket.connect() 
without testing each time.  This may be premature optimization, but my feeling is that 
it may be worth 
it.

Mike

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

DO NOT REPLY [Bug 28322] - Connection timeout logic redesign

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=28322

Connection timeout logic redesign





--- Additional Comments From [EMAIL PROTECTED]  2004-04-09 23:08 ---
Created an attachment (id=11204)
Patch (take 1)

-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]