Bug 46985 - Impossible condition in coyote.http11.Http11Processor.process(Socket socket)
Impossible condition in coyote.http11.Http11Processor.process(Socket socket)
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Catalina
unspecified
PC Windows XP
: P2 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2009-04-07 16:36 UTC by Sebb
Modified: 2009-05-26 14:39 UTC (History)
0 users



Attachments
Patch to correct Javadoc and remove dead code. It is for tc6.0.x. (1.53 KB, patch)
2009-05-18 15:25 UTC, Konstantin Kolinko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebb 2009-04-07 16:36:50 UTC
coyote.http11.Http11Processor.process(Socket socket)
...

        int soTimeout = socket.getSoTimeout();
        int oldSoTimeout = soTimeout;

        int threadRatio = (endpoint.getCurrentThreadsBusy() * 100)
                / endpoint.getMaxThreads();
        if (threadRatio > 75) {
            keepAliveLeft = 1;
        }
        
        if (soTimeout != oldSoTimeout) {

The above condition will never be true. It looks like the code is trying to reset the timeout if it has not changed, but it will never do so.

Both the method and the class have a variable called "socket" which may be part of the problem - is the method trying to set the instance socket to have the same timeout as the parameter socket, or vice versa?

The socket parameter should be renamed.

Note that the Javadoc appears to be completely wrong as well.
Comment 1 Mark Thomas 2009-04-08 08:00:59 UTC
Thanks for the reminder. Cleaning up this was on my todo list post fixing bug 46666.

I have fixed trunk and proposed the fix for 6.0.x
Comment 2 Konstantin Kolinko 2009-05-18 15:10:29 UTC
For reference: Mark's fix in trunk is r763262

(In reply to comment #0)
> coyote.http11.Http11Processor.process(Socket socket)
> ...
> 
>         int soTimeout = socket.getSoTimeout();
>         int oldSoTimeout = soTimeout;
> 
>         int threadRatio = (endpoint.getCurrentThreadsBusy() * 100)
>                 / endpoint.getMaxThreads();
>         if (threadRatio > 75) {
>             keepAliveLeft = 1;
>         }
> 
>         if (soTimeout != oldSoTimeout) {
> 
> The above condition will never be true. It looks like the code is trying to
> reset the timeout if it has not changed, but it will never do so.
> 

That "if (soTimeout != oldSoTimeout) { .. }" works in TC 5.5, because of some block of code that precedes it, but is dead in TC 6.0 because that preceding block is removed. 

You can look at TC 5.5 sources, but as I was studying it through svn log, I will give a reference to annotated source of it at revision 398045:

http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java?annotate=398045&limit_changes=0&pathrev=423920

The code was the following:

751 : 	  	  	 int soTimeout = socket.getSoTimeout(); 
752 : 	  	  	 int oldSoTimeout = soTimeout;
753 : 	  	  	
754 : 	remm 	396579 	int threadRatio = (endpoint.getCurrentThreadsBusy() * 100)
755 : 	remm 	389146 	/ endpoint.getMaxThreads();
756 : 	  	  	if ((threadRatio > 33) && (threadRatio <= 66)) {
757 : 	  	  	soTimeout = soTimeout / 2;
758 : 	  	  	} else if ((threadRatio > 66) && (threadRatio <= 90)) {
759 : 	  	  	soTimeout = soTimeout / 3;
760 : 	  	  	keepAliveLeft = 1;
761 : 	  	  	} else if (threadRatio > 90) {
762 : 	  	  	soTimeout = soTimeout / 20;
763 : 	  	  	keepAliveLeft = 1;
764 : 	  	  	}
765 : 	  	  	
766 : 	  	  	if (soTimeout != oldSoTimeout) {

I will propose removal of the dead code in TC 6.0.

> Both the method and the class have a variable called "socket" which may be part
> of the problem - is the method trying to set the instance socket to have the
> same timeout as the parameter socket, or vice versa?
> 
> The socket parameter should be renamed.
> 

No error there, no need to rename.

Both are pointing to the same object. Also, you may note, that
this.socket is set back to null at the end of the method.

> Note that the Javadoc appears to be completely wrong as well.

Oh, it says about input and output streams. Those are provided by the socket.
Comment 3 Konstantin Kolinko 2009-05-18 15:25:19 UTC
Created attachment 23684 [details]
Patch to correct Javadoc and remove dead code. It is for tc6.0.x.

I will propose the patch for TC 6.0.
For record: this issue does not exist in 5.5.
Comment 4 Sebb 2009-05-18 17:07:32 UTC
(In reply to comment #2)
> That "if (soTimeout != oldSoTimeout) { .. }" works in TC 5.5, because of some
> block of code that precedes it, but is dead in TC 6.0 because that preceding
> block is removed. 

OK, I see - the condition did once mean something.
 
> > Both the method and the class have a variable called "socket" which may be part
> > of the problem - is the method trying to set the instance socket to have the
> > same timeout as the parameter socket, or vice versa?
> > 
> > The socket parameter should be renamed.
> > 
> 
> No error there, no need to rename.

Not an error, but it's very confusing to use the same name for a parameter and an instance variable.

Using the same name is just about OK in short ctors or one-line set() methods, but elsewhere it's not helpful.

> Both are pointing to the same object. Also, you may note, that
> this.socket is set back to null at the end of the method.

Huh? The socket parameter may or may not be the same as the instance variable.

I don't see either being set to null.

> 
> > Note that the Javadoc appears to be completely wrong as well.
> 
> Oh, it says about input and output streams. Those are provided by the socket.

Indeed
Comment 5 Konstantin Kolinko 2009-05-18 17:26:26 UTC
(In reply to comment #4)
> 
> I don't see either being set to null.
> 

    public void process(Socket socket)
        throws IOException {
(..)
        // Setting up the I/O
        this.socket = socket;
        inputBuffer.setInputStream(socket.getInputStream());
        outputBuffer.setOutputStream(socket.getOutputStream());

(.....)

        // Recycle
        inputBuffer.recycle();
        outputBuffer.recycle();
        this.socket = null;
        // Recycle ssl info
        sslSupport = null;
    }

I slightly wonder why there is no try/finally...
Comment 6 Mark Thomas 2009-05-26 14:39:21 UTC
This is fixed in 6.0.x and will be included in 6.0.21 onwards.