You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bi...@apache.org on 2003/09/12 05:51:36 UTC
cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
billbarker 2003/09/11 20:51:36
Modified: util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
Log:
Make certain that we don't return a bad Socket.
Fix for Bug #21763
Reported By: Rex Young rex.young@fmr.com
Reported By: wendy69 ro@k00l.net
Revision Changes Path
1.16 +4 -4 jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net/PoolTcpEndpoint.java
Index: PoolTcpEndpoint.java
===================================================================
RCS file: /home/cvs/jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net/PoolTcpEndpoint.java,v
retrieving revision 1.15
retrieving revision 1.16
diff -u -r1.15 -r1.16
--- PoolTcpEndpoint.java 26 Jul 2003 15:04:35 -0000 1.15
+++ PoolTcpEndpoint.java 12 Sep 2003 03:51:36 -0000 1.16
@@ -389,12 +389,12 @@
if (accepted != null) {
try {
accepted.close();
- accepted = null;
} catch(Exception ex) {
msg = sm.getString("endpoint.err.nonfatal",
accepted, ex);
log.warn(msg, ex);
}
+ accepted = null;
}
if( ! running ) return null;
[5.0.12] Delay
Posted by Remy Maucherat <re...@apache.org>.
I won't tag 5.0.12 until early next week, because:
- we're currently examining and testing fixes for bug 21763; two people
have volunteered to test the recent changes, and confirm the problem is
fixed
- I'm working on adding additional buffering features in Coyote HTTP;
the change is not very extensive, so I hope there will not be any regression
A 4.1.28 release should also be made soon, incorporating the latest fixes.
Remy
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
Posted by Stefan Bodewig <bo...@apache.org>.
On Fri, 12 Sep 2003, Bill Barker <wb...@wilshire.com> wrote:
> From: "Stefan Bodewig" <bo...@apache.org>
>>
>> wouldn't it be better to put the "accepted = null" into a finally
>> block
>
> Wouldn't do anything. The 'accepted' variable is local to the
> stack-frame, so it goes away if I throw clear of the method.
OK, thanks. I just looked at the commit mail as I suspect that one of
our customer production systems gets bitten by the bug - I didn't look
at the complete code. Makes sense, then.
> I briefly thought about changing the catch to 'Throwable', but is it
> really possible for Socket.close to throw anything other than an
> Exception?
Everything is possible ;-)
I have no idea what the Socket implementation does if shutdown(2) sets
errno to ENOTCONN for example (I'd guess, throw a plain
SocketException).
I don't think that non-Exceptions are too likely to happen and if they
do it probably happens in a situation where you can't recover anyway
(OutOfMemory, StackOverflow, ThreadDeath ...). Catching Throwable may
be the savest thing to do.
Stefan
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net
PoolTcpEndpoint.java
Posted by Remy Maucherat <re...@apache.org>.
Bill Barker wrote:
>>I don't know. From traces I saw, there are conditions where there could
>>be no thread listening on the socket (at that point, the connector was
>>dead, of course), while everything else in the TP was looking ok
>>(including no deadlock). There didn't seem to be anything in the logs
>>related to an error during accept.
>
> If you max out the threads, then the thread that is waiting in ThreadPool is
> the one that will do the next accept. It's supposed to be a temporary
> condition (since a thread should get returned to to Pool soon). Porting the
> connectionTimeout throttling back to 4.1 should make it more responsive in
> this case (since Tomcat will stop doing keep-alives). Of course, what it's
> really telling you is that your maxProcessors is set too low :).
I haven't been able to measure the effects of this, so I am unsure of
the usefulness of the feature. Anyone having conducted more serious
testing with TC 5 yet ?
Remy
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
Posted by Bill Barker <wb...@wilshire.com>.
----- Original Message -----
From: "Remy Maucherat" <re...@apache.org>
To: "Tomcat Developers List" <to...@jakarta.apache.org>
Sent: Friday, September 12, 2003 1:38 AM
Subject: Re: cvs commit:
jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net
PoolTcpEndpoint.java
> Bill Barker wrote:
> >>On 12 Sep 2003, <bi...@apache.org> wrote:
> >>
> >>
> >>> +++ PoolTcpEndpoint.java 12 Sep 2003 03:51:36 -0000 1.16
> >>> @@ -389,12 +389,12 @@
> >>> if (accepted != null) {
> >>> try {
> >>> accepted.close();
> >>> - accepted = null;
> >>> } catch(Exception ex) {
> >>> msg = sm.getString("endpoint.err.nonfatal",
> >>> accepted, ex);
> >>> log.warn(msg, ex);
> >>> }
> >>> + accepted = null;
> >>> }
> >>>
> >>> if( ! running ) return null;
> >>
> >>wouldn't it be better to put the "accepted = null" into a finally
> >>block so you clean up even in the (unlikely but possible) case where
> >>close throws an Error (or Throwable) instead of an Exception?
> >
> > Wouldn't do anything. The 'accepted' variable is local to the
stack-frame,
> > so it goes away if I throw clear of the method. In this case you just
get a
> > DoS condition where no threads are listening on the ServerSocket. I
briefly
> > thought about changing the catch to 'Throwable', but is it really
possible
> > for Socket.close to throw anything other than an Exception?
>
> I don't know. From traces I saw, there are conditions where there could
> be no thread listening on the socket (at that point, the connector was
> dead, of course), while everything else in the TP was looking ok
> (including no deadlock). There didn't seem to be anything in the logs
> related to an error during accept.
If you max out the threads, then the thread that is waiting in ThreadPool is
the one that will do the next accept. It's supposed to be a temporary
condition (since a thread should get returned to to Pool soon). Porting the
connectionTimeout throttling back to 4.1 should make it more responsive in
this case (since Tomcat will stop doing keep-alives). Of course, what it's
really telling you is that your maxProcessors is set too low :).
>
> I'd catch throwable just to be safe, personally :)
I'll catch throwable then.
>
> Remy
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: tomcat-dev-help@jakarta.apache.org
>
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net
PoolTcpEndpoint.java
Posted by Remy Maucherat <re...@apache.org>.
Bill Barker wrote:
>>On 12 Sep 2003, <bi...@apache.org> wrote:
>>
>>
>>> +++ PoolTcpEndpoint.java 12 Sep 2003 03:51:36 -0000 1.16
>>> @@ -389,12 +389,12 @@
>>> if (accepted != null) {
>>> try {
>>> accepted.close();
>>> - accepted = null;
>>> } catch(Exception ex) {
>>> msg = sm.getString("endpoint.err.nonfatal",
>>> accepted, ex);
>>> log.warn(msg, ex);
>>> }
>>> + accepted = null;
>>> }
>>>
>>> if( ! running ) return null;
>>
>>wouldn't it be better to put the "accepted = null" into a finally
>>block so you clean up even in the (unlikely but possible) case where
>>close throws an Error (or Throwable) instead of an Exception?
>
> Wouldn't do anything. The 'accepted' variable is local to the stack-frame,
> so it goes away if I throw clear of the method. In this case you just get a
> DoS condition where no threads are listening on the ServerSocket. I briefly
> thought about changing the catch to 'Throwable', but is it really possible
> for Socket.close to throw anything other than an Exception?
I don't know. From traces I saw, there are conditions where there could
be no thread listening on the socket (at that point, the connector was
dead, of course), while everything else in the TP was looking ok
(including no deadlock). There didn't seem to be anything in the logs
related to an error during accept.
I'd catch throwable just to be safe, personally :)
Remy
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
Posted by Bill Barker <wb...@wilshire.com>.
----- Original Message -----
From: "Stefan Bodewig" <bo...@apache.org>
To: <to...@jakarta.apache.org>
Sent: Friday, September 12, 2003 12:58 AM
Subject: Re: cvs commit:
jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net
PoolTcpEndpoint.java
> On 12 Sep 2003, <bi...@apache.org> wrote:
>
> > +++ PoolTcpEndpoint.java 12 Sep 2003 03:51:36 -0000 1.16
> > @@ -389,12 +389,12 @@
> > if (accepted != null) {
> > try {
> > accepted.close();
> > - accepted = null;
> > } catch(Exception ex) {
> > msg = sm.getString("endpoint.err.nonfatal",
> > accepted, ex);
> > log.warn(msg, ex);
> > }
> > + accepted = null;
> > }
> >
> > if( ! running ) return null;
>
> wouldn't it be better to put the "accepted = null" into a finally
> block so you clean up even in the (unlikely but possible) case where
> close throws an Error (or Throwable) instead of an Exception?
>
Wouldn't do anything. The 'accepted' variable is local to the stack-frame,
so it goes away if I throw clear of the method. In this case you just get a
DoS condition where no threads are listening on the ServerSocket. I briefly
thought about changing the catch to 'Throwable', but is it really possible
for Socket.close to throw anything other than an Exception?
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: tomcat-dev-help@jakarta.apache.org
>
Re: cvs commit: jakarta-tomcat-connectors/util/java/org/apache/tomcat/util/net PoolTcpEndpoint.java
Posted by Stefan Bodewig <bo...@apache.org>.
On 12 Sep 2003, <bi...@apache.org> wrote:
> +++ PoolTcpEndpoint.java 12 Sep 2003 03:51:36 -0000 1.16
> @@ -389,12 +389,12 @@
> if (accepted != null) {
> try {
> accepted.close();
> - accepted = null;
> } catch(Exception ex) {
> msg = sm.getString("endpoint.err.nonfatal",
> accepted, ex);
> log.warn(msg, ex);
> }
> + accepted = null;
> }
>
> if( ! running ) return null;
wouldn't it be better to put the "accepted = null" into a finally
block so you clean up even in the (unlikely but possible) case where
close throws an Error (or Throwable) instead of an Exception?
Stefan