BindingServiceProperties.bindings regressed to a non-thread-safe TreeMap in GH-3158: concurrent first use of binding names corrupts it
Maintainers usually reply within 1 day
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 62/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Active
- Tech stack
- java
- Domain
- backend, stream-processing
Research direction
Start with the bindings field and bindIfNecessary() in BindingServiceProperties, then follow the getBindingDestination, getBindingProperties, getProducerProperties and getConsumerProperties callers. Keep the field's type and comparator in mind, since the fix must stay thread-safe and case-insensitive. Add the multi-threaded reproduction from the issue as a test in the binding properties test class. Done means that test passes, the key-count check matches, and case-insensitive lookups and sorted iteration still work.
Written by the indexing model from the issue text.
Description
BindingServiceProperties.bindings has been a plain TreeMap again since GH-3158 (845a3e58). That map is written to on read, from many threads, with no synchronization, so concurrent first use of binding names corrupts it. This is the defect fixed in #3011, brought back. I mentioned it in a comment on #3158, but that issue is closed, so I am opening this one for tracking.
What goes wrong
getBindingDestination(), getBindingProperties(), getProducerProperties() and getConsumerProperties() all call bindIfNecessary(), which, for a name that is not yet in the map, runs bindToDefault() and does this.bindings.put(...):
private void bindIfNecessary(String bindingName) {
if (!this.bindings.containsKey(bindingName)) {
this.bindToDefault(bindingName); // ... this.bindings.put(binding, target)
}
}
Spring Cloud Stream itself calls these from arbitrary threads, with no lock: StreamBridge.send(), BindingService, AbstractMessageChannelBinder, FunctionConfiguration, MessageConverterConfigurer, ChannelsEndpoint (and BindingsLifecycleController calls setBindings). Any application that publishes with StreamBridge from several threads, or starts bindings concurrently, can hit it. Applications cannot work around it by locking on their side, because Spring's own callers do not take that lock.
An unsynchronized TreeMap that is mutated concurrently is corrupted in the ways TreeMap documents: lost entries, size() no longer matching the number of entries, NullPointerException in TreeMap.rotateRight (the failure reported in #3011), or an iteration that never ends.
Observed in production
Two incidents on Spring Cloud Stream 4.3.3 (the line numbers in the stack traces match that version):
- One thread stays
RUNNABLEinTreeMap.successor(once underBindingServiceProperties.setBindings->putAll) and never returns. About 40 and 140 other threads, respectively, then block behind it, thread pools starve and stop advancing until the pod is restarted. - A heap dump of the stuck JVM shows the map's
sizefield at 288 while only 66 entries are reachable fromroot, and a parent/child cycle between two nodes. In the second dump, 94 declared and 93 reachable, with two nodes cross-linked and three wrong parent pointers.
Reproduction
No broker is needed. This fails on a TreeMap and passes with a thread-safe map:
GenericApplicationContext context = new GenericApplicationContext();
context.refresh();
BindingServiceProperties properties = new BindingServiceProperties();
properties.setApplicationContext(context);
int threads = 16, operations = 300, names = 400;
CountDownLatch start = new CountDownLatch(1);
ExecutorService executor = Executors.newFixedThreadPool(threads);
List<Future<?>> futures = new ArrayList<>();
for (int t = 0; t < threads; t++) {
Random random = new Random(t);
futures.add(executor.submit(() -> {
start.await();
for (int i = 0; i < operations; i++) {
String name = "binding-" + random.nextInt(names);
properties.getBindingDestination(name);
properties.getBindingProperties(name);
properties.getProducerProperties(name);
properties.getConsumerProperties(name + "-in");
}
return null;
}));
}
start.countDown();
for (Future<?> f : futures) {
f.get(30, TimeUnit.SECONDS); // a spinning thread shows up as a TimeoutException
}
int iterated = 0;
for (String ignored : properties.getBindings().keySet()) {
iterated++;
}
assertThat(iterated).isEqualTo(properties.getBindings().size()); // fails on TreeMap, e.g. expected: 798 but was: 800
History of the field
| Commit | Date | Change |
|---|---|---|
| fd20513 (#3011) | 2024-09-26 | TreeMap -> ConcurrentSkipListMap, to fix a NullPointerException in TreeMap.rotateRight called from StreamBridge.send |
| afc8fa2 (#3076) | 2025-02-26 | ConcurrentSkipListMap -> ConcurrentHashMap: "Using 'ConcurrentSkipListMap' was causing significant performance issues" |
| 845a3e5 (#3158) | 2025-12-01 | ConcurrentHashMap -> TreeMap(String.CASE_INSENSITIVE_ORDER), because ConcurrentHashMap made binding names case-sensitive |
I checked the bytecode of the released jars. The map is a TreeMap (not thread-safe) in 4.3.1, 4.3.2, 4.3.3, 5.0.1, 5.0.2, 5.0.3, 5.0.4, 5.1.0-M1 and 5.1.0-RC1, and on main. It is a ConcurrentSkipListMap in 4.2.0 and a ConcurrentHashMap in 4.2.1 to 4.3.0 and 5.0.0.
So the two earlier fixes each traded one problem for another: the ConcurrentHashMap is thread-safe but case-sensitive, and the TreeMap is case-insensitive but not thread-safe.
The skip list was not shown to be slow
#3076 is the reason the ConcurrentSkipListMap was dropped, but I could not find anything in that thread that shows the map was the cause:
- The reporter tried the
ConcurrentHashMapbuild (4.2.1-SNAPSHOT) and reported "I do not see improvements with4.2.1-SNAPSHOT. I see same performance as4.2.0". - They traced the slowdown to a Spring Boot commit that changed child-context creation (spring-projects/spring-boot@09fdb9d), and later found their
output-bindingsproperty was underspring.cloud.configinstead ofspring.cloud.stream, so every firstgetBinder()call went throughbindToDefault()(~4 ms, which is theBinderwork, not the map). They closed it after optimizing their own binder.
For the per-lookup cost I measured the three candidates in a plain timing loop (not JMH; 300 entries, ~29-character keys, Java 21, warmed up, read-only):
| Map | get, 1 thread |
get, 16 threads |
|---|---|---|
TreeMap(CASE_INSENSITIVE_ORDER) |
~72 ns | ~152 ns |
ConcurrentSkipListMap(CASE_INSENSITIVE_ORDER) |
~188 ns | ~381 ns |
ConcurrentHashMap (case-sensitive) |
~5 ns | ~11 ns |
That is about 0.1 µs more per lookup than the TreeMap, and a binding is only added once, so I would not expect it to matter for start-up or for sending.
Proposal
Use new ConcurrentSkipListMap<>(String.CASE_INSENSITIVE_ORDER). It is thread-safe, keeps case-insensitive lookup, the original key casing and sorted iteration (so what #3158 needs), and needs no other change. A ConcurrentHashMap cannot be used because it is case-sensitive.
I have a one-line patch with a test (the reproduction above) and can open a pull request.
- Dominant language
- Java
- Stars
- 1.1k
- Forks
- 651
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 17
Getting set up
This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from spring-cloud/spring-cloud-stream
-
Difficulty 3/5 1-2 days Newbie friendliness 74/100
spring-cloud/spring-cloud-stream#3316 ·
Maintainers usually reply within 1 day
-
StreamBridge removing BindingProperties leads to wrong destination in ProvisioningProviderMay be free again @olegz claimed this 56 days ago, and no pull request is open. Openbug
spring-cloud/spring-cloud-stream#3257 · 6 comments · 2 reactions · 1 assignee ·
Maintainers usually reply within 1 day
-
StreamBridge's hashProducerProperties produces hash collisions across different binding names, causing Partition key cannot be nullMay be free again A pull request for this issue was closed without being merged. Open
Difficulty 4/5 3-5 days Newbie friendliness 48/100
spring-cloud/spring-cloud-stream#3242 · 2 comments ·
Maintainers usually reply within 1 day
-
v4.3.3 no longer adds a header "TIMESTAMP" on kafka messages consumedMay be free again @olegz claimed this 87 days ago, and no pull request is open. OpenBackport 4.3.x bug
spring-cloud/spring-cloud-stream#3211 · 3 comments · 1 assignee ·
Maintainers usually reply within 1 day
-
Clarify support for manual ack/nack with batch mode + DLQ; potential bug in acknowledge(index) pathOpen
Difficulty 4/5 3-5 days Newbie friendliness 48/100
spring-cloud/spring-cloud-stream#3189 · 3 comments · 1 reaction ·
Maintainers usually reply within 1 day
All issues in spring-cloud/spring-cloud-stream
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
Maintainers usually reply within 3 days
-
A proxy that fails to start logs a RejectedExecutionException stack trace from its shutdown hookOpenfollow-up
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
Sanne/incus-spawn#1953 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
atlassian/data-center-helm-charts#1170 ·
Maintainers usually reply within 1 day
-
Bug
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
Maintainers usually reply within 1 day
-
Issue: Bug
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
OpenAPITools/openapi-generator#25524 ·
Maintainers usually reply within 1 day