Fix thread safety in dissect processor by eliminating mutable shared state by shik-25 · Pull Request #6858 · opensearch-project/data-prepper

shik-25 · 2026-05-13T08:27:17Z

Description

Fixes a thread-safety bug in the dissect processor by eliminating shared mutable state in Dissector. Previously, dissectText() mutated shared Delimiter.start/end and Field.value on every invocation, causing data races when multiple threads processed events concurrently. The @SingleThread annotation was a workaround that limited throughput unnecessarily.

Fix

dissectText() now uses local int[] arrays for delimiter positions and a local Map<Field, String> for field values instead of mutating shared objects.
Template field objects (normalFieldMap, indirectFieldMap, etc.) are read-only after construction and wrapped in Collections.unmodifiableMap().
dissectText() returns Map<String, String> directly, replacing the separate getDissectedFields() call.
@SingleThread is removed.

Issues Resolved

Resolves #5546

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…state Signed-off-by: Shikhar Dhawale <shikhar_dhawale@apple.com>

shik-25 · 2026-05-13T08:31:10Z

Benchmark: GC allocation pressure before vs after thread-safety fix

Setup: JMH, @Threads(4) (shared Dissector instance across 4 concurrent threads), gc profiler, 2 forks × 5 iterations × 10s

Metric	Before	After	Delta
Alloc/op (`gc.alloc.rate.norm`)	2336 B	2744 B	+408 B (+17.5%)
Alloc rate (`gc.alloc.rate`)	4287 MB/s	5373 MB/s	+25%
GC time/cycle	0.48ms	0.46ms	-4%
Throughput	1,924,895 ops/s	2,053,606 ops/s	+6.7%

What changed: The new code allocates local int[] arrays for delimiter positions and a Map<Field, String> for field values on every dissectText() call instead of mutating shared objects. These are short-lived allocations collected by the young gen immediately.

Verdict: The +408 bytes/op is the honest cost of the fix. The +25% allocation rate increase is proportional - 6.7% more throughput × 17.5% more per op. GC pause time per cycle is slightly lower (-4%), meaning the JVM handles the extra allocation efficiently. Throughput improves because shared-state contention is eliminated under concurrent execution

shik-25 · 2026-05-13T08:39:30Z

-    public List<Field> getDissectedFields(){
-        final List<Field> dissectedFields = new ArrayList<>();
-        Map<String, AppendField> appendFieldMap = getAppendedFields(unAppendedFieldsMap);
+    private Map<String, String> getDissectedFields(Map<Field, String> localValues) {


Design note on Map<Field, String> localValues:

localValues uses Field objects as map keys without overriding
equals/hashCode, relying on identity semantics. This is intentional:

AppendField allows multiple instances with the same logical key (e.g., {+field1} appearing twice in a pattern). Implementing equals/hashCode by key would cause silent overwrites in localValues.

Field objects are never copied or rebuilt — the same instances from normalFieldMap, indirectFieldMap, etc. are used for both put and get, so identity-based lookup is always correct.

Identity semantics are the right tradeoff here.

dlvenable

Thank you @shik-25 for this contribution! This looks like a good improvement, but I do see a possible increase in memory pressure from your benchmarks.

I left a few comments on the design and some ideas to try to reduce memory pressure as well.

dlvenable · 2026-05-21T14:16:03Z

@@ -12,13 +12,8 @@
 public abstract class Field {
    boolean stripTrailing = false;
    private String key;


Let's remove setKey and instead make a package-protected constructor. Right now, only the inheriting constructors (e.g. AppendField) call setKey. We can pass it in the constructor and also make the field final to ensure this doesn't get used incorrectly.

dlvenable · 2026-05-21T14:26:07Z

            if(fieldString==null) {
                return;
            }
            Field field = fieldHelper.getField(fieldString);


I think this code should become something like this:

Field lastField = fieldsList.size() > 0 ? fieldsList.getLast() : null; Field field = fieldHelper.getField(fieldString, lastField); fieldsList.addLast(field);

You add a nullable Field to the FieldHelper. This has the benefit of allow us to make Field::setNext() package protected.

Also, you can add a check on setField so that it throws if it is called after having received a next value. This can help us find subtle errors that may be introduced.

dlvenable · 2026-05-21T14:29:41Z

-        } catch (Exception e) {
-            return false;
+            if (head == null) {
+                return null;


This is a good defensive check, but maybe it could hide some subtle errors. Perhaps add a good ERROR log about this.

dlvenable · 2026-05-21T14:30:28Z

    useJUnitPlatform()
+}
+
+tasks.withType(Zip).configureEach {


Why did you add this?

dlvenable · 2026-05-21T14:33:54Z

+        @Setup
+        public void setUp() {
+            dissector = new Dissector("%{timestamp} %{level} %{message}");
+            input = "2024-01-15 ERROR service crashed";


It would probably be ideal to also have a benchmark that includes more scenarios like append fields, indirect fields, and strip-trailing. But this isn't necessary for the PR.

dlvenable · 2026-05-21T14:42:00Z

+        if (!computeDelimiterPositions(text, delimStarts, delimEnds, n)) {
+            return null;
+        }
+        Map<Field, String> localValues = new HashMap<>(); // keyed by identity — same Field instances used for put and get


I believe you can set this with an initial size: new HashMap(delimiterList.size()). This may help in some cases to reduce memory and allocations.

dlvenable · 2026-05-21T14:45:32Z

-        final List<Field> dissectedFields = new ArrayList<>();
-        Map<String, AppendField> appendFieldMap = getAppendedFields(unAppendedFieldsMap);
+    private Map<String, String> getDissectedFields(Map<Field, String> localValues) {
+        final Map<String, String> results = new HashMap<>();


This also looks like you can gauge the size in advance:

new HashMap<>(normalFieldMap.size() + appendFieldMap.size() + unAppendedFieldsMap.size())

There are some conditions on null for normalFieldMap. But this is probably OK.

dlvenable · 2026-05-21T14:49:41Z

+        for (Map.Entry<String, List<AppendField>> entry : unAppendedFieldsMap.entrySet()) {
+            List<AppendField> copy = new ArrayList<>(entry.getValue());
+            Collections.sort(copy);
+            String value = copy.stream()


The copy up to this point is going to be the same for all iterations since it is based on unAppendedFieldsMap. Can we also pre-sort this in the constructor?

Fix thread safety in dissect processor by eliminating mutable shared …

a1777b2

…state Signed-off-by: Shikhar Dhawale <shikhar_dhawale@apple.com>

shik-25 requested review from KarstenSchnitter, Zhangxunmt, dinujoh, divbok, dlvenable, graytaylor0, kkondaka, oeyh, san81, sb2k16 and srikanthjg as code owners May 13, 2026 08:27

shik-25 commented May 13, 2026

View reviewed changes

dlvenable requested changes May 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix thread safety in dissect processor by eliminating mutable shared state#6858

Fix thread safety in dissect processor by eliminating mutable shared state#6858
shik-25 wants to merge 1 commit into
opensearch-project:mainfrom
shik-25:fix/dissect-thread-safety

shik-25 commented May 13, 2026

Uh oh!

shik-25 commented May 13, 2026

Uh oh!

shik-25 May 13, 2026

Uh oh!

dlvenable left a comment

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

dlvenable May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shik-25 commented May 13, 2026

Description

Fix

Issues Resolved

Check List

Uh oh!

shik-25 commented May 13, 2026

Benchmark: GC allocation pressure before vs after thread-safety fix

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlvenable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants