Delay pipe attachment for builder-invoked responses to prevent race condition

[SanojPunchihewa]

Purpose

Delay pipe attachment for builder-invoked responses to prevent race condition

When multiple chunks are received by the TargetHandler, the target response may read from the pipe and mark consumer output as active. This can trigger the SourceResponse to start writing the response while the worker thread is still executing setContentType() and adding response headers.

Fixes: wso2/product-integrator-mi#4938

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2fb77af2-8d14-46fd-a444-0dd679f07db3

📥 Commits

Reviewing files that changed from the base of the PR and between 67ee70a and 2edc482.

📒 Files selected for processing (1)

• modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/PassThroughHttpSender.java

---

📝 Walkthrough

Changes Summary

This PR addresses a race condition in HTTP response handling by deferring pipe attachment for builder-invoked responses.

What Changed

In the submitResponse method of PassThroughHttpSender, the timing of pipe attachment and connection to the source response has been made conditional:

1. Earlier attachment deferred: The immediate calls to pipe.attachConsumer(conn) and sourceResponse.connect(pipe) are now skipped when both conditions are true:

   • The message builder was invoked (MESSAGE_BUILDER_INVOKED is true)
   • There is an entity body to serialize (not marked with NO_ENTITY_BODY)

2. Delayed attachment: The pipe attachment and connection are now performed later in the response-writing path, immediately before formatting and serializing the response. This ensures:

   • pipe.attachConsumer(conn) is called after all headers have been set
   • sourceResponse.connect(pipe) is established after content type and other headers are finalized
   • The formatter writes the response to the pipe after the pipe is properly connected

3. Unchanged for other cases: For responses without an entity body or when the message builder was not invoked, the original behavior is preserved.

Root Cause

When chunked responses arrive at the backend, subsequent chunks can trigger pipe.produce() which calls consumerIoControl.requestOutput(). This schedules an NIO reactor thread to write the response while the worker thread is still executing setContentType() and addHeader() operations. These operations mutate the unsynchronized headers HashMap, causing a ConcurrentModificationException. By deferring pipe attachment until after header initialization is complete, concurrent access to the headers map is prevented.

Impact

• Lines changed: +9/-2
• Only affects response serialization paths where the message builder is invoked
• Prevents concurrent unsynchronized access to response headers during multi-threaded request processing

Walkthrough

In PassThroughHttpSender.submitResponse, a delayPipeAttachment boolean is introduced. When MESSAGE_BUILDER_INVOKED is true and noEntityBody is false, the early unconditional calls to pipe.attachConsumer(conn) and sourceResponse.connect(pipe) are skipped. Those same two calls are then added at the entry point of the response body-serialization branch, so attachment occurs immediately before response formatting and writing rather than before setContentType mutates the headers map.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete. While it identifies the purpose and the linked issue, it omits most required sections from the repository template.	Add the missing required sections: Goals, Approach, Release note, Documentation, Automation tests (unit and integration), Security checks, and Test environment.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: delaying pipe attachment for builder-invoked responses to prevent a race condition.
Linked Issues check	✅ Passed	The PR implementation directly addresses the race condition by deferring pipe attachment and connection until after headers are initialized, preventing concurrent mutation of the headers HashMap.
Out of Scope Changes check	✅ Passed	All changes in PassThroughHttpSender.java are scoped to addressing the race condition in the linked issue. No out-of-scope modifications are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}