On 14/09/2015 11:42, Konstantin Kolinko wrote: > 2015-09-14 12:47 GMT+03:00 <ma...@apache.org>: >> Author: markt >> Date: Mon Sep 14 09:47:50 2015 >> New Revision: 1702887 >> >> URL: http://svn.apache.org/r1702887 >> Log: >> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=58370 >> Fix data races when executor is being shut down >> >> Modified: >> tomcat/tc8.0.x/trunk/ (props changed) >> >> tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/AbstractEndpoint.java >> tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml >> > >> Modified: >> tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/AbstractEndpoint.java >> URL: >> http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/AbstractEndpoint.java?rev=1702887&r1=1702886&r2=1702887&view=diff >> ============================================================================== >> --- >> tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/AbstractEndpoint.java >> (original) >> +++ >> tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/AbstractEndpoint.java >> Mon Sep 14 09:47:50 2015 > > >> @@ -576,8 +581,10 @@ public abstract class AbstractEndpoint<S >> } >> >> public void shutdownExecutor() { >> - if ( executor!=null && internalExecutor ) { >> - if ( executor instanceof ThreadPoolExecutor ) { >> + Executor executor = this.executor; >> + this.executor = null; >> + if (executor != null && internalExecutor) { >> + if (executor instanceof ThreadPoolExecutor) { >> //this is our internal one, so we need to shut it down >> ThreadPoolExecutor tpe = (ThreadPoolExecutor) executor; >> tpe.shutdownNow(); >> @@ -595,7 +602,6 @@ public abstract class AbstractEndpoint<S >> TaskQueue queue = (TaskQueue) tpe.getQueue(); >> queue.setParent(null); >> } >> - executor = null; >> } >> } >> > > > In the old implementation of shutdownExecutor() the "executor = null;" > assignment is conditioned by internalExecutor flag. In the new > implementation it is assigned unconditionally. > > I think this change in behaviour is wrong. If it is an external > executor (internalExecutor flag is false), this reference should not > be nulled. shutdownExecutor() is called from stopInternal() of > NioEndpoint etc., so in theory it is possible to call start() on > endpoint if it has not been destroy()ed yet. Nulling the reference > changes behaviour of such start().
Yep. Good catch. I'll get that fixed. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
