This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8ce20a2011e8d0b8980561c2f694d12c39068232 Author: opencode <[email protected]> AuthorDate: Tue Oct 20 10:44:26 2026 +0200 Synchronize access to the bindings of a naming context The bindings of a NamingContext are held in a HashMap that is also shared with the views of the context which a lookup of the empty name returns. Application threads read that map while they look resources up, while other threads bind and unbind resources, for example when NamingContextListener reacts to the JMX notifications that are sent while a web application is started or stopped. As the map was used without any synchronization, list() and listBindings() regularly failed with a ConcurrentModificationException, and the map itself could be damaged by a write which resized it while another thread was reading it, which loses bindings and can make a binding that is there invisible to a reader. Synchronize every access to the map itself on the map object. The map is final and it is the same object in the views of the context, so this also covers the paths which go through a view. Lookups of a sub-context and the creation of the object which a resource reference points at are left outside the lock, so no user code runs while the lock is held. The enumeration which list() and listBindings() return now walks a copy of the entries which was taken under the lock, rather than the live view of the values of the map, which is what threw the exception when the map was written to while the enumeration was still in progress. In unbind(), an entry is now read and removed under the same lock; for a name which denotes a binding in a sub-context, the call into that sub-context is made once the lock is released. The requirement that the map is only used under its own lock is now documented on the field, since the field is protected and the constructor which takes the map is public. --- java/org/apache/naming/NamingContext.java | 71 ++++++++--- .../naming/TestNamingContextConcurrency.java | 137 +++++++++++++++++++++ 2 files changed, 193 insertions(+), 15 deletions(-) diff --git a/java/org/apache/naming/NamingContext.java b/java/org/apache/naming/NamingContext.java index 2b9c104dde..5fa94b81c5 100644 --- a/java/org/apache/naming/NamingContext.java +++ b/java/org/apache/naming/NamingContext.java @@ -16,9 +16,11 @@ */ package org.apache.naming; +import java.util.ArrayList; import java.util.Enumeration; import java.util.HashMap; import java.util.Hashtable; +import java.util.List; import javax.naming.Binding; import javax.naming.CompositeName; @@ -113,7 +115,10 @@ public class NamingContext implements Context { /** - * Bindings in this Context. + * Bindings in this Context. The map is shared with the views of this context that a lookup of the empty name + * returns and it is read by the threads performing lookups while resources are bound and unbound on admin + * threads, so every access to the map itself must be synchronized on it. The {@link NamingEntry} values carry + * their own lock for the creation of singleton objects. */ protected final HashMap<String,NamingEntry> bindings; @@ -200,20 +205,26 @@ public class NamingContext implements Context { throw new NamingException(sm.getString("namingContext.invalidName")); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + boolean leaf; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + leaf = name.size() == 1; + if (leaf) { + bindings.remove(name.get(0)); + } + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); } - if (name.size() > 1) { + if (!leaf) { if (entry.type == NamingEntry.CONTEXT) { ((Context) entry.value).unbind(name.getSuffix(1)); } else { throw new NamingException(sm.getString("namingContext.contextExpected", name.get(0))); } - } else { - bindings.remove(name.get(0)); } } @@ -246,10 +257,17 @@ public class NamingContext implements Context { name = name.getSuffix(1); } if (name.isEmpty()) { - return new NamingContextEnumeration(bindings.values().iterator()); + List<NamingEntry> entries; + synchronized (bindings) { + entries = new ArrayList<>(bindings.values()); + } + return new NamingContextEnumeration(entries.iterator()); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); @@ -275,10 +293,17 @@ public class NamingContext implements Context { name = name.getSuffix(1); } if (name.isEmpty()) { - return new NamingContextBindingsEnumeration(bindings.values().iterator(), this); + List<NamingEntry> entries; + synchronized (bindings) { + entries = new ArrayList<>(bindings.values()); + } + return new NamingContextBindingsEnumeration(entries.iterator(), this); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); @@ -311,7 +336,10 @@ public class NamingContext implements Context { throw new NamingException(sm.getString("namingContext.invalidName")); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); @@ -326,7 +354,9 @@ public class NamingContext implements Context { } else { if (entry.type == NamingEntry.CONTEXT) { ((Context) entry.value).close(); - bindings.remove(name.get(0)); + synchronized (bindings) { + bindings.remove(name.get(0)); + } } else { throw new NotContextException(sm.getString("namingContext.contextExpected", name.get(0))); } @@ -384,7 +414,10 @@ public class NamingContext implements Context { return nameParser; } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); } @@ -493,7 +526,10 @@ public class NamingContext implements Context { return new NamingContext(env, this.name, bindings); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (entry == null) { throw new NameNotFoundException(sm.getString("namingContext.nameNotBound", name, name.get(0))); @@ -604,7 +640,10 @@ public class NamingContext implements Context { throw new NamingException(sm.getString("namingContext.invalidName")); } - NamingEntry entry = bindings.get(name.get(0)); + NamingEntry entry; + synchronized (bindings) { + entry = bindings.get(name.get(0)); + } if (name.size() > 1) { if (entry == null) { @@ -638,7 +677,9 @@ public class NamingContext implements Context { } else { entry = new NamingEntry(name.get(0), toBind, NamingEntry.ENTRY); } - bindings.put(name.get(0), entry); + synchronized (bindings) { + bindings.put(name.get(0), entry); + } } } diff --git a/test/org/apache/naming/TestNamingContextConcurrency.java b/test/org/apache/naming/TestNamingContextConcurrency.java new file mode 100644 index 0000000000..00243fc0cf --- /dev/null +++ b/test/org/apache/naming/TestNamingContextConcurrency.java @@ -0,0 +1,137 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.naming; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import javax.naming.Binding; +import javax.naming.NameClassPair; +import javax.naming.NamingEnumeration; + +import org.junit.Assert; +import org.junit.Test; + +/* + * The bindings of a NamingContext are read by the threads performing lookups while admin + * threads bind and unbind resources, as NamingContextListener does on a JMX notification. + * Before the map accesses were synchronized, this regularly led to ConcurrentModification- + * Exception in list() and listBindings() and could lose updates to the underlying HashMap. + */ +public class TestNamingContextConcurrency { + + @Test + public void testConcurrentBindUnbindAndListing() throws Exception { + // A context that is not registered with the ContextAccessController is writable + NamingContext context = new NamingContext(null, "test"); + // The context returned for the empty name shares the bindings map and its lock + NamingContext view = (NamingContext) context.lookup(""); + + int stableEntries = 200; + for (int i = 0; i < stableEntries; i++) { + context.bind("stable" + i, "value" + i); + } + + AtomicBoolean stop = new AtomicBoolean(false); + AtomicReference<Throwable> failure = new AtomicReference<>(); + AtomicInteger writes = new AtomicInteger(); + + Thread writer = new Thread(() -> { + int i = 0; + try { + while (!stop.get()) { + String name = "dynamic" + (i % 50); + context.bind(name, "value" + i); + context.unbind(name); + writes.incrementAndGet(); + i++; + } + } catch (Throwable t) { + failure.compareAndSet(null, t); + } + }); + + List<Thread> readers = new ArrayList<>(); + for (int r = 0; r < 4; r++) { + readers.add(new Thread(() -> { + try { + while (!stop.get()) { + NamingEnumeration<NameClassPair> names = context.list(""); + int count = 0; + while (names.hasMore()) { + names.next(); + count++; + } + // The stable entries are never removed, so they must all be there + Assert.assertTrue("Missing stable entries in list()", count >= stableEntries); + NamingEnumeration<Binding> bindings = context.listBindings(""); + while (bindings.hasMore()) { + bindings.next(); + } + view.list(""); + view.lookup("stable0"); + context.lookup("stable0"); + try { + Thread.sleep(20); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } catch (Throwable t) { + failure.compareAndSet(null, t); + } + })); + } + + writer.start(); + for (Thread reader : readers) { + reader.start(); + } + Thread.sleep(2000); + stop.set(true); + writer.join(30000); + for (Thread reader : readers) { + reader.join(30000); + } + + Assert.assertFalse("The writer did not terminate.", writer.isAlive()); + for (Thread reader : readers) { + Assert.assertFalse("A reader did not terminate.", reader.isAlive()); + } + Throwable seen = failure.get(); + Assert.assertNull("Concurrent access failed: " + seen, seen); + Assert.assertTrue("The writer made no progress", writes.get() > 0); + + // The map must still be intact: all stable entries present, no dynamic leftovers + NamingEnumeration<Binding> remaining = context.listBindings(""); + int stableCount = 0; + int dynamicCount = 0; + while (remaining.hasMore()) { + Binding binding = remaining.next(); + if (binding.getName().startsWith("stable")) { + stableCount++; + } else if (binding.getName().startsWith("dynamic")) { + dynamicCount++; + } + } + Assert.assertEquals(stableEntries, stableCount); + Assert.assertEquals("Leftover dynamic entries", 0, dynamicCount); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
