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.
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
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.
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.
(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
(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...
This is fixed in 6.0.x and will be included in 6.0.21 onwards.