On 04/01/2012 13:59, Konstantin Kolinko wrote: > 2012/1/4 Konstantin Kolinko <knst.koli...@gmail.com>: >> >> 2. The >> processSocket(desc[n*2+1], SocketStatus.DISCONNECT); >> call just above the fixed line. >> >> It looks like a NOOP, because processSocket(long,SocketStatus) has an >> if() that does not mention SocketStatus.DISCONNECT. >> >> I think it is one more way to leak sockets. This method is used only >> in comet requests. > > and I think that if() might be a cause of missing some ERROR events in comet.
Agreed. >> Can the "if()" condition in processSocket(long,SocketStatus) be removed? >> Besides SocketStatus.DISCONNECT. there is one more status that is not >> mentioned in that if() and can lead to similar leaks: >> SocketStatus.ERROR. >> >> The if() was added in >> http://svn.apache.org/viewvc?view=revision&revision=944518 >> >> I do not see a reason for this if(). The r944518 says about "running >> with a SecurityManager" so it should have just added a >> PrivilegedAction there. >> > > One more oddity with r944518 in AprEndpoint: > 1. I do not understand why one has to set TCCL before calling > getExecutor().execute(). We do not do this in other places. E.g. we do > not do it in AprEndpoint#processSocket(long). I suspect because the Servlet 3 Async code was re-using bits of the Comet code (inconsistently in different ways on different connectors). > 2. r944518 says about TCK failures, but TCK does not test comet and so > it could not be a reason for the change in this method which is used > only by comet. It is only used by Comet now but r944518 pre-dates r1001698 where there was a major refactoring of the connectors. > So it looks that not only the if(), but the whole r944518 change in > AprEndpoint has to be reverted. That looks OK to me. I'll do that but run the TCKs as a double check. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
