Filip Hanik - Dev Lists wrote: > I'm generally against this find bugs 'may be bugs' issues. > is there an actual bug here?
Reported bug, no. Bugs uses could hit, yes. Hence why this is in trunk and not being proposed for backport. Are all the syncs necessary? I haven't looked in detail but I suspect that right now, that most of them are not. As we increase JMX functionality and have more dynamic configuration then we'll almost certainly need them so I don't see the harm in getting this right now. Mark > > Filip > > ma...@apache.org wrote: >> Author: markt >> Date: Wed Apr 8 16:08:42 2009 >> New Revision: 763298 >> >> URL: http://svn.apache.org/viewvc?rev=763298&view=rev >> Log: >> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=46990 >> Various sync issues. >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/core/StandardContext.java >> tomcat/trunk/java/org/apache/catalina/core/StandardHost.java >> >> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >> tomcat/trunk/java/org/apache/catalina/util/InstanceSupport.java >> tomcat/trunk/java/org/apache/catalina/util/LifecycleSupport.java >> >> Modified: tomcat/trunk/java/org/apache/catalina/core/StandardContext.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardContext.java?rev=763298&r1=763297&r2=763298&view=diff >> >> ============================================================================== >> >> --- tomcat/trunk/java/org/apache/catalina/core/StandardContext.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/core/StandardContext.java >> Wed Apr 8 16:08:42 2009 >> @@ -201,6 +201,8 @@ >> * application, in the order they were encountered in the web.xml >> file. >> */ >> private String applicationListeners[] = new String[0]; >> + + private final Object applicationListenersLock = new Object(); >> >> >> /** >> @@ -223,6 +225,8 @@ >> private ApplicationParameter applicationParameters[] = >> new ApplicationParameter[0]; >> >> + private final Object applicationParametersLock = new Object(); >> + >> /** >> * The application available flag for this Context. >> @@ -263,6 +267,8 @@ >> * The security constraints for this web application. >> */ >> private SecurityConstraint constraints[] = new >> SecurityConstraint[0]; >> + + private final Object constraintsLock = new Object(); >> >> >> /** >> @@ -364,6 +370,9 @@ >> * defined in the deployment descriptor. >> */ >> private FilterMap filterMaps[] = new FilterMap[0]; >> + + private final Object filterMapsLock = new Object(); >> + >> >> /** >> * Filter mappings added via {...@link ServletContext} may have to >> be inserted >> @@ -388,6 +397,8 @@ >> */ >> private String instanceListeners[] = new String[0]; >> >> + private final Object instanceListenersLock = new Object(); >> + >> >> /** >> * The login configuration descriptor for this web application. >> @@ -508,6 +519,8 @@ >> */ >> private String securityRoles[] = new String[0]; >> >> + private final Object securityRolesLock = new Object(); >> + >> >> /** >> * The servlet mappings for this web application, keyed by >> @@ -515,6 +528,8 @@ >> */ >> private HashMap<String, String> servletMappings = >> new HashMap<String, String>(); >> + + private final Object servletMappingsLock = new Object(); >> >> >> /** >> @@ -559,12 +574,16 @@ >> */ >> private String watchedResources[] = new String[0]; >> >> + private final Object watchedResourcesLock = new Object(); >> + >> >> /** >> * The welcome files for this application. >> */ >> private String welcomeFiles[] = new String[0]; >> >> + private final Object welcomeFilesLock = new Object(); >> + >> >> /** >> * The set of classnames of LifecycleListeners that will be added >> @@ -572,6 +591,7 @@ >> */ >> private String wrapperLifecycles[] = new String[0]; >> >> + private final Object wrapperLifecyclesLock = new Object(); >> >> /** >> * The set of classnames of ContainerListeners that will be added >> @@ -579,6 +599,7 @@ >> */ >> private String wrapperListeners[] = new String[0]; >> >> + private final Object wrapperListenersLock = new Object(); >> >> /** >> * The pathname to the work directory for this context (relative to >> @@ -2021,7 +2042,7 @@ >> */ >> public void addApplicationListener(String listener) { >> >> - synchronized (applicationListeners) { >> + synchronized (applicationListenersLock) { >> String results[] =new String[applicationListeners.length >> + 1]; >> for (int i = 0; i < applicationListeners.length; i++) { >> if (listener.equals(applicationListeners[i])) { >> @@ -2048,7 +2069,7 @@ >> */ >> public void addApplicationParameter(ApplicationParameter >> parameter) { >> >> - synchronized (applicationParameters) { >> + synchronized (applicationParametersLock) { >> String newName = parameter.getName(); >> for (int i = 0; i < applicationParameters.length; i++) { >> if >> (newName.equals(applicationParameters[i].getName()) && >> @@ -2145,7 +2166,7 @@ >> } >> >> // Add this constraint to the set for our web application >> - synchronized (constraints) { >> + synchronized (constraintsLock) { >> SecurityConstraint results[] = >> new SecurityConstraint[constraints.length + 1]; >> for (int i = 0; i < constraints.length; i++) >> @@ -2231,7 +2252,7 @@ >> >> validateFilterMap(filterMap); >> // Add this filter mapping to our registered set >> - synchronized (filterMaps) { >> + synchronized (filterMapsLock) { >> FilterMap results[] =new FilterMap[filterMaps.length + 1]; >> System.arraycopy(filterMaps, 0, results, 0, >> filterMaps.length); >> results[filterMaps.length] = filterMap; >> @@ -2256,7 +2277,7 @@ >> validateFilterMap(filterMap); >> >> // Add this filter mapping to our registered set >> - synchronized (filterMaps) { >> + synchronized (filterMapsLock) { >> FilterMap results[] = new FilterMap[filterMaps.length + 1]; >> System.arraycopy(filterMaps, 0, results, 0, >> filterMapInsertPoint); >> results[filterMapInsertPoint] = filterMap; >> @@ -2313,7 +2334,7 @@ >> */ >> public void addInstanceListener(String listener) { >> >> - synchronized (instanceListeners) { >> + synchronized (instanceListenersLock) { >> String results[] =new String[instanceListeners.length + 1]; >> for (int i = 0; i < instanceListeners.length; i++) >> results[i] = instanceListeners[i]; >> @@ -2456,7 +2477,7 @@ >> */ >> public void addSecurityRole(String role) { >> >> - synchronized (securityRoles) { >> + synchronized (securityRolesLock) { >> String results[] =new String[securityRoles.length + 1]; >> for (int i = 0; i < securityRoles.length; i++) >> results[i] = securityRoles[i]; >> @@ -2507,7 +2528,7 @@ >> (sm.getString("standardContext.servletMap.pattern", >> pattern)); >> >> // Add this mapping to our registered set >> - synchronized (servletMappings) { >> + synchronized (servletMappingsLock) { >> String name2 = servletMappings.get(pattern); >> if (name2 != null) { >> // Don't allow more than one servlet on the same pattern >> @@ -2551,7 +2572,7 @@ >> */ >> public void addWatchedResource(String name) { >> >> - synchronized (watchedResources) { >> + synchronized (watchedResourcesLock) { >> String results[] = new String[watchedResources.length + 1]; >> for (int i = 0; i < watchedResources.length; i++) >> results[i] = watchedResources[i]; >> @@ -2570,7 +2591,7 @@ >> */ >> public void addWelcomeFile(String name) { >> >> - synchronized (welcomeFiles) { >> + synchronized (welcomeFilesLock) { >> // Welcome files from the application deployment descriptor >> // completely replace those from the default conf/web.xml >> file >> if (replaceWelcomeFiles) { >> @@ -2597,7 +2618,7 @@ >> */ >> public void addWrapperLifecycle(String listener) { >> >> - synchronized (wrapperLifecycles) { >> + synchronized (wrapperLifecyclesLock) { >> String results[] =new String[wrapperLifecycles.length + 1]; >> for (int i = 0; i < wrapperLifecycles.length; i++) >> results[i] = wrapperLifecycles[i]; >> @@ -2617,7 +2638,7 @@ >> */ >> public void addWrapperListener(String listener) { >> >> - synchronized (wrapperListeners) { >> + synchronized (wrapperListenersLock) { >> String results[] =new String[wrapperListeners.length + 1]; >> for (int i = 0; i < wrapperListeners.length; i++) >> results[i] = wrapperListeners[i]; >> @@ -2649,7 +2670,7 @@ >> wrapper = new StandardWrapper(); >> } >> >> - synchronized (instanceListeners) { >> + synchronized (instanceListenersLock) { >> for (int i = 0; i < instanceListeners.length; i++) { >> try { >> Class<?> clazz = >> Class.forName(instanceListeners[i]); >> @@ -2663,7 +2684,7 @@ >> } >> } >> >> - synchronized (wrapperLifecycles) { >> + synchronized (wrapperLifecyclesLock) { >> for (int i = 0; i < wrapperLifecycles.length; i++) { >> try { >> Class<?> clazz = >> Class.forName(wrapperLifecycles[i]); >> @@ -2678,7 +2699,7 @@ >> } >> } >> >> - synchronized (wrapperListeners) { >> + synchronized (wrapperListenersLock) { >> for (int i = 0; i < wrapperListeners.length; i++) { >> try { >> Class<?> clazz = Class.forName(wrapperListeners[i]); >> @@ -2713,7 +2734,9 @@ >> */ >> public ApplicationParameter[] findApplicationParameters() { >> >> - return (applicationParameters); >> + synchronized (applicationParametersLock) { >> + return (applicationParameters); >> + } >> >> } >> >> @@ -2829,7 +2852,9 @@ >> */ >> public String[] findInstanceListeners() { >> >> - return (instanceListeners); >> + synchronized (instanceListenersLock) { >> + return (instanceListeners); >> + } >> >> } >> >> @@ -2987,7 +3012,7 @@ >> */ >> public boolean findSecurityRole(String role) { >> >> - synchronized (securityRoles) { >> + synchronized (securityRolesLock) { >> for (int i = 0; i < securityRoles.length; i++) { >> if (role.equals(securityRoles[i])) >> return (true); >> @@ -3004,7 +3029,9 @@ >> */ >> public String[] findSecurityRoles() { >> >> - return (securityRoles); >> + synchronized (securityRolesLock) { >> + return (securityRoles); >> + } >> >> } >> >> @@ -3017,7 +3044,7 @@ >> */ >> public String findServletMapping(String pattern) { >> >> - synchronized (servletMappings) { >> + synchronized (servletMappingsLock) { >> return (servletMappings.get(pattern)); >> } >> >> @@ -3030,7 +3057,7 @@ >> */ >> public String[] findServletMappings() { >> >> - synchronized (servletMappings) { >> + synchronized (servletMappingsLock) { >> String results[] = new String[servletMappings.size()]; >> return >> (servletMappings.keySet().toArray(results)); >> @@ -3113,7 +3140,7 @@ >> */ >> public boolean findWelcomeFile(String name) { >> >> - synchronized (welcomeFiles) { >> + synchronized (welcomeFilesLock) { >> for (int i = 0; i < welcomeFiles.length; i++) { >> if (name.equals(welcomeFiles[i])) >> return (true); >> @@ -3129,7 +3156,9 @@ >> * defined, a zero length array will be returned. >> */ >> public String[] findWatchedResources() { >> - return watchedResources; >> + synchronized (watchedResourcesLock) { >> + return watchedResources; >> + } >> } >> @@ -3139,7 +3168,9 @@ >> */ >> public String[] findWelcomeFiles() { >> >> - return (welcomeFiles); >> + synchronized (welcomeFilesLock) { >> + return (welcomeFiles); >> + } >> >> } >> >> @@ -3150,7 +3181,9 @@ >> */ >> public String[] findWrapperLifecycles() { >> >> - return (wrapperLifecycles); >> + synchronized (wrapperLifecyclesLock) { >> + return (wrapperLifecycles); >> + } >> >> } >> >> @@ -3161,7 +3194,9 @@ >> */ >> public String[] findWrapperListeners() { >> >> - return (wrapperListeners); >> + synchronized (wrapperListenersLock) { >> + return (wrapperListeners); >> + } >> >> } >> >> @@ -3220,7 +3255,7 @@ >> */ >> public void removeApplicationListener(String listener) { >> >> - synchronized (applicationListeners) { >> + synchronized (applicationListenersLock) { >> >> // Make sure this welcome file is currently present >> int n = -1; >> @@ -3260,7 +3295,7 @@ >> */ >> public void removeApplicationParameter(String name) { >> >> - synchronized (applicationParameters) { >> + synchronized (applicationParametersLock) { >> >> // Make sure this parameter is currently present >> int n = -1; >> @@ -3319,7 +3354,7 @@ >> */ >> public void removeConstraint(SecurityConstraint constraint) { >> >> - synchronized (constraints) { >> + synchronized (constraintsLock) { >> >> // Make sure this constraint is currently present >> int n = -1; >> @@ -3399,7 +3434,7 @@ >> */ >> public void removeFilterMap(FilterMap filterMap) { >> >> - synchronized (filterMaps) { >> + synchronized (filterMapsLock) { >> >> // Make sure this filter mapping is currently present >> int n = -1; >> @@ -3438,7 +3473,7 @@ >> */ >> public void removeInstanceListener(String listener) { >> >> - synchronized (instanceListeners) { >> + synchronized (instanceListenersLock) { >> >> // Make sure this welcome file is currently present >> int n = -1; >> @@ -3550,7 +3585,7 @@ >> */ >> public void removeSecurityRole(String role) { >> >> - synchronized (securityRoles) { >> + synchronized (securityRolesLock) { >> >> // Make sure this security role is currently present >> int n = -1; >> @@ -3589,7 +3624,7 @@ >> public void removeServletMapping(String pattern) { >> >> String name = null; >> - synchronized (servletMappings) { >> + synchronized (servletMappingsLock) { >> name = servletMappings.remove(pattern); >> } >> Wrapper wrapper = (Wrapper) findChild(name); >> @@ -3624,7 +3659,7 @@ >> */ >> public void removeWatchedResource(String name) { >> - synchronized (watchedResources) { >> + synchronized (watchedResourcesLock) { >> >> // Make sure this watched resource is currently present >> int n = -1; >> @@ -3661,7 +3696,7 @@ >> */ >> public void removeWelcomeFile(String name) { >> >> - synchronized (welcomeFiles) { >> + synchronized (welcomeFilesLock) { >> >> // Make sure this welcome file is currently present >> int n = -1; >> @@ -3701,7 +3736,7 @@ >> public void removeWrapperLifecycle(String listener) { >> >> >> - synchronized (wrapperLifecycles) { >> + synchronized (wrapperLifecyclesLock) { >> >> // Make sure this welcome file is currently present >> int n = -1; >> @@ -3740,7 +3775,7 @@ >> public void removeWrapperListener(String listener) { >> >> >> - synchronized (wrapperListeners) { >> + synchronized (wrapperListenersLock) { >> >> // Make sure this welcome file is currently present >> int n = -1; >> @@ -4701,7 +4736,9 @@ >> // Notify our interested LifecycleListeners >> lifecycle.fireLifecycleEvent(DESTROY_EVENT, null); >> >> - instanceListeners = new String[0]; >> + synchronized (instanceListenersLock) { >> + instanceListeners = new String[0]; >> + } >> >> } >> >> Modified: tomcat/trunk/java/org/apache/catalina/core/StandardHost.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardHost.java?rev=763298&r1=763297&r2=763298&view=diff >> >> ============================================================================== >> >> --- tomcat/trunk/java/org/apache/catalina/core/StandardHost.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/core/StandardHost.java Wed >> Apr 8 16:08:42 2009 >> @@ -72,6 +72,8 @@ >> * The set of aliases for this Host. >> */ >> private String[] aliases = new String[0]; >> + + private final Object aliasesLock = new Object(); >> >> >> /** >> @@ -514,20 +516,19 @@ >> >> alias = alias.toLowerCase(); >> >> - // Skip duplicate aliases >> - for (int i = 0; i < aliases.length; i++) { >> - if (aliases[i].equals(alias)) >> - return; >> + synchronized (aliasesLock) { >> + // Skip duplicate aliases >> + for (int i = 0; i < aliases.length; i++) { >> + if (aliases[i].equals(alias)) >> + return; >> + } >> + // Add this alias to the list >> + String newAliases[] = new String[aliases.length + 1]; >> + for (int i = 0; i < aliases.length; i++) >> + newAliases[i] = aliases[i]; >> + newAliases[aliases.length] = alias; >> + aliases = newAliases; >> } >> - >> - // Add this alias to the list >> - String newAliases[] = new String[aliases.length + 1]; >> - for (int i = 0; i < aliases.length; i++) >> - newAliases[i] = aliases[i]; >> - newAliases[aliases.length] = alias; >> - >> - aliases = newAliases; >> - >> // Inform interested listeners >> fireContainerEvent(ADD_ALIAS_EVENT, alias); >> >> @@ -556,7 +557,9 @@ >> */ >> public String[] findAliases() { >> >> - return (this.aliases); >> + synchronized (aliasesLock) { >> + return (this.aliases); >> + } >> >> } >> >> @@ -631,7 +634,7 @@ >> >> alias = alias.toLowerCase(); >> >> - synchronized (aliases) { >> + synchronized (aliasesLock) { >> >> // Make sure this alias is currently present >> int n = -1; >> @@ -766,7 +769,9 @@ >> } >> >> public String[] getAliases() { >> - return aliases; >> + synchronized (aliasesLock) { >> + return aliases; >> + } >> } >> >> private boolean initialized=false; >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java?rev=763298&r1=763297&r2=763298&view=diff >> >> ============================================================================== >> >> --- >> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >> (original) >> +++ >> tomcat/trunk/java/org/apache/catalina/tribes/membership/Membership.java >> Wed Apr 8 16:08:42 2009 >> @@ -41,6 +41,8 @@ >> { >> protected static final MemberImpl[] EMPTY_MEMBERS = new >> MemberImpl[0]; >> + private final Object membersLock = new Object(); >> + /** >> * The name of this membership, has to be the same as the name >> for the local >> * member >> @@ -63,7 +65,7 @@ >> protected Comparator<Member> memberComparator = new >> MemberComparator(); >> >> public Object clone() { >> - synchronized (members) { >> + synchronized (membersLock) { >> Membership clone = new Membership(local, memberComparator); >> clone.map = (HashMap<MemberImpl, MbrEntry>) map.clone(); >> clone.members = new MemberImpl[members.length]; >> @@ -139,7 +141,7 @@ >> * @param member The member to add >> */ >> public synchronized MbrEntry addMember(MemberImpl member) { >> - synchronized (members) { >> + synchronized (membersLock) { >> MbrEntry entry = new MbrEntry(member); >> if (!map.containsKey(member) ) { >> map.put(member, entry); >> @@ -160,7 +162,7 @@ >> */ >> public void removeMember(MemberImpl member) { >> map.remove(member); >> - synchronized (members) { >> + synchronized (membersLock) { >> int n = -1; >> for (int i = 0; i < members.length; i++) { >> if (members[i] == member || members[i].equals(member)) { >> >> Modified: tomcat/trunk/java/org/apache/catalina/util/InstanceSupport.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/util/InstanceSupport.java?rev=763298&r1=763297&r2=763298&view=diff >> >> ============================================================================== >> >> --- tomcat/trunk/java/org/apache/catalina/util/InstanceSupport.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/util/InstanceSupport.java >> Wed Apr 8 16:08:42 2009 >> @@ -64,6 +64,8 @@ >> * The set of registered InstanceListeners for event notifications. >> */ >> private InstanceListener listeners[] = new InstanceListener[0]; >> + + private final Object listenersLock = new Object(); // Lock >> object for changes to listeners >> >> >> /** >> @@ -95,7 +97,7 @@ >> */ >> public void addInstanceListener(InstanceListener listener) { >> >> - synchronized (listeners) { >> + synchronized (listenersLock) { >> InstanceListener results[] = >> new InstanceListener[listeners.length + 1]; >> for (int i = 0; i < listeners.length; i++) >> @@ -312,7 +314,7 @@ >> */ >> public void removeInstanceListener(InstanceListener listener) { >> >> - synchronized (listeners) { >> + synchronized (listenersLock) { >> int n = -1; >> for (int i = 0; i < listeners.length; i++) { >> if (listeners[i] == listener) { >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/util/LifecycleSupport.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/util/LifecycleSupport.java?rev=763298&r1=763297&r2=763298&view=diff >> >> ============================================================================== >> >> --- tomcat/trunk/java/org/apache/catalina/util/LifecycleSupport.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/util/LifecycleSupport.java >> Wed Apr 8 16:08:42 2009 >> @@ -66,6 +66,8 @@ >> * The set of registered LifecycleListeners for event notifications. >> */ >> private LifecycleListener listeners[] = new LifecycleListener[0]; >> + + private final Object listenersLock = new Object(); // Lock >> object for changes to listeners >> >> >> // --------------------------------------------------------- >> Public Methods >> @@ -78,7 +80,7 @@ >> */ >> public void addLifecycleListener(LifecycleListener listener) { >> >> - synchronized (listeners) { >> + synchronized (listenersLock) { >> LifecycleListener results[] = >> new LifecycleListener[listeners.length + 1]; >> for (int i = 0; i < listeners.length; i++) >> @@ -126,7 +128,7 @@ >> */ >> public void removeLifecycleListener(LifecycleListener listener) { >> >> - synchronized (listeners) { >> + synchronized (listenersLock) { >> int n = -1; >> for (int i = 0; i < listeners.length; i++) { >> if (listeners[i] == listener) { >> >> >> >> --------------------------------------------------------------------- >> 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
