Re: svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-14 Thread Mark Thomas 
On 13/06/2012 23:22, Konstantin Kolinko wrote:
 2012/6/13  ma...@apache.org:
 Author: markt
 Date: Wed Jun 13 18:29:07 2012
 New Revision: 1349984

 URL: http://svn.apache.org/viewvc?rev=1349984view=rev
 Log:
 Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53173
 Remove some duplicate calls to countDownConnection()
 While the connector is running, destroySocket() will call 
 countDownConnection()
 Once the connector is stopped, the latch is removed so it does not matter 
 that destroySocket() does not call countDownConnection() in that case
 
 
 It seems that this commit has not been merged to 7.0 yet.

I'll do that shortly.

 Looking at AprEndpoint#processSocketWithOptions(..) it looks that it
 should return false when running flag is false. It does not matter
 with the current code though.
 
 
 Once the connector is stopped, the latch is removed so it does not matter 
 that destroySocket() does not call countDownConnection() in that case
 
 Maybe mention the above rationale in a comment in 
 AprEndpoint#destroySocket(..)?

+1

 In my understanding the reason why we do not countdown here is that it
 is known that no new connections will be allowed, so there is no need
 to maintain the counter that limits new connections.

Exactly. The code goes further and removes the latch as soon as stop is
called.

 It might be nice to amend test cases to check that connection counter
 goes back to 0 after Tomcat shuts down, but it probably never happens.
 Skipping countdown in destroySocket is one reason why such a test may
 return false results.

Could be tricky given that the latch is removed.

 Rainer mentioned that he observed Incorrect connection count warning
 with BIO connector, but only once (in
 tcn-tc-ant_test-bio.out-1.1.24.sles10.x86_64:1). The cause is still
 unknown.

Yep - still on the todo list but not as urgent and not blocking a 7.0.28
release.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-13 Thread markt 
Author: markt
Date: Wed Jun 13 18:29:07 2012
New Revision: 1349984

URL: http://svn.apache.org/viewvc?rev=1349984view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53173
Remove some duplicate calls to countDownConnection()
While the connector is running, destroySocket() will call countDownConnection()
Once the connector is stopped, the latch is removed so it does not matter that 
destroySocket() does not call countDownConnection() in that case

Modified:
tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java?rev=1349984r1=1349983r2=1349984view=diff
==
--- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Wed Jun 13 
18:29:07 2012
@@ -981,12 +981,10 @@ public class AprEndpoint extends Abstrac
 if (running  !paused) {
 // Hand this socket off to an appropriate processor
 if (!processSocketWithOptions(socket)) {
-countDownConnection();
 // Close socket and pool right away
 destroySocket(socket);
 }
 } else {
-countDownConnection();
 // Close socket and pool right away
 destroySocket(socket);
 }



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

RE: svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-13 Thread Filip Hanik (mailing lists) 


  } else {
 -countDownConnection();
  // Close socket and pool right away
  destroySocket(socket);
  }
 

[Filip Hanik] 
'running' variable could be false here, at which point, socket nor countdown 
happens




 
 
 -
 To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: dev-h...@tomcat.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

RE: svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-13 Thread Filip Hanik (mailing lists) 
My suggestion for a 7.0.28 is to have -1 for the default value of 
maxConnections for APR. This disables maxConnections until we have a better 
grip on it.

Changing it to track closures is quite a surgery, something I would do for 
trunk, but leave out of 7.0.x. This leaves existing code flow intact, but 
changes parameter types everywhere.

Actually rewriting the code to ensure it never gets called more than once 
without tracking it, seems a bit risky. 

I ran unit tests with the same fix, 
 -Original Message-
 From: ma...@apache.org [mailto:ma...@apache.org]
 Sent: Wednesday, June 13, 2012 12:29 PM
 To: dev@tomcat.apache.org
 Subject: svn commit: r1349984 -
 /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
 
 Author: markt
 Date: Wed Jun 13 18:29:07 2012
 New Revision: 1349984
 
 URL: http://svn.apache.org/viewvc?rev=1349984view=rev
 Log:
 Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53173
 Remove some duplicate calls to countDownConnection()
 While the connector is running, destroySocket() will call
 countDownConnection()
 Once the connector is stopped, the latch is removed so it does not
 matter that destroySocket() does not call countDownConnection() in that
 case
 
 Modified:
 tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
 
 Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
 URL:
 http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/ne
 t/AprEndpoint.java?rev=1349984r1=1349983r2=1349984view=diff
 
 ==
 --- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java
 (original)
 +++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Wed
 Jun 13 18:29:07 2012
 @@ -981,12 +981,10 @@ public class AprEndpoint extends Abstrac
  if (running  !paused) {
  // Hand this socket off to an appropriate
 processor
  if (!processSocketWithOptions(socket)) {
 -countDownConnection();
  // Close socket and pool right away
  destroySocket(socket);
  }
  } else {
 -countDownConnection();
  // Close socket and pool right away
  destroySocket(socket);
  }
 
 
 
 -
 To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: dev-h...@tomcat.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-13 Thread Mark Thomas 
On 13/06/2012 22:14, Filip Hanik (mailing lists) wrote:
 
 
  } else {
 -countDownConnection();
  // Close socket and pool right away
  destroySocket(socket);
  }

 
 [Filip Hanik] 
 'running' variable could be false here, at which point, socket nor countdown 
 happens

Already addressed. Read the commit comment.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1349984 - /tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java

2012-06-13 Thread Konstantin Kolinko 
2012/6/13  ma...@apache.org:
 Author: markt
 Date: Wed Jun 13 18:29:07 2012
 New Revision: 1349984

 URL: http://svn.apache.org/viewvc?rev=1349984view=rev
 Log:
 Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53173
 Remove some duplicate calls to countDownConnection()
 While the connector is running, destroySocket() will call 
 countDownConnection()
 Once the connector is stopped, the latch is removed so it does not matter 
 that destroySocket() does not call countDownConnection() in that case


It seems that this commit has not been merged to 7.0 yet.

Looking at AprEndpoint#processSocketWithOptions(..) it looks that it
should return false when running flag is false. It does not matter
with the current code though.


 Once the connector is stopped, the latch is removed so it does not matter 
 that destroySocket() does not call countDownConnection() in that case

Maybe mention the above rationale in a comment in AprEndpoint#destroySocket(..)?

In my understanding the reason why we do not countdown here is that it
is known that no new connections will be allowed, so there is no need
to maintain the counter that limits new connections.


It might be nice to amend test cases to check that connection counter
goes back to 0 after Tomcat shuts down, but it probably never happens.
Skipping countdown in destroySocket is one reason why such a test may
return false results.


Rainer mentioned that he observed Incorrect connection count warning
with BIO connector, but only once (in
tcn-tc-ant_test-bio.out-1.1.24.sles10.x86_64:1). The cause is still
unknown.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org