Skip to content

fix(triple): clean up REST form request decoder - #16412

Open
juzi050 wants to merge 3 commits into
apache:3.3from
juzi050:fix/triple-rest-post-decoder-cleanup
Open

fix(triple): clean up REST form request decoder#16412
juzi050 wants to merge 3 commits into
apache:3.3from
juzi050:fix/triple-rest-post-decoder-cleanup

Conversation

@juzi050

@juzi050 juzi050 commented Aug 1, 2026

Copy link
Copy Markdown

What is the purpose of the change?

Closes #16403.

When Triple REST parses an application/x-www-form-urlencoded or multipart/form-data request, DefaultHttpRequest lazily creates and caches an HttpPostRequestDecoder. The decoder was not destroyed after the request completed, so DefaultHttpDataFactory continued to retain the request and its form data.

This change attaches decoder cleanup to the HTTP response lifecycle:

  • Add a dedicated method to destroy the decoder cached by DefaultHttpRequest.
  • Register decoder cleanup with both HTTP/1.1 and HTTP/2 server response observers.
  • Clean up after normal or error response termination, while ensuring cleanup runs only once when multiple terminal signals are received.
  • If the transport closes before an asynchronous unary invocation completes normally, defer cleanup until the invocation completes so multipart data remains available to the service method during request processing.

How was it tested?

Extended HttpPostRequestDecoderLifecycleTest to cover the real Triple REST request path:

  • Verify that a form request no longer remains tracked by DefaultHttpDataFactory after normal completion.
  • Verify decoder cleanup through the HTTP/1.1 response observer.
  • Verify that HTTP/2 remote cancellation does not release multipart data while an asynchronous service invocation is still using it, and that the data is released after the invocation completes.
  • Verify that custom request adapters that do not use DefaultHttpRequest do not require decoder cleanup.

Added AbstractServerHttpChannelObserverTest to verify that transport closure defers cleanup until normal application completion and that cleanup runs only once when multiple terminal signals are received. The targeted tests passed.

Commands:

./mvnw -pl dubbo-remoting/dubbo-remoting-http12 -Dtest=AbstractServerHttpChannelObserverTest test
./mvnw -pl dubbo-rpc/dubbo-rpc-triple -am -Dtest=HttpPostRequestDecoderLifecycleTest -Dsurefire.failIfNoSpecifiedTests=false test

The changed files were also formatted with codestyle/dubbo_codestyle_for_idea.xml. Maven Checkstyle and Spotless checks passed.

Checklist

  • Make sure there is a GitHub issue for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how it does it, and why it does it.
  • Write the necessary unit tests to verify the code.
  • Make sure GitHub Actions can pass.

@codecov-commenter

codecov-commenter commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.04%. Comparing base (3a30432) to head (b931dc5).

Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #16412      +/-   ##
============================================
+ Coverage     60.87%   61.04%   +0.16%     
+ Complexity    11766    11765       -1     
============================================
  Files          1953     1953              
  Lines         89273    89304      +31     
  Branches      13473    13478       +5     
============================================
+ Hits          54346    54514     +168     
+ Misses        29333    29183     -150     
- Partials       5594     5607      +13     
Flag Coverage Δ
integration-tests-java21 32.15% <45.45%> (+0.01%) ⬆️
integration-tests-java8 32.20% <45.45%> (-0.01%) ⬇️
samples-tests-java21 32.13% <0.00%> (-0.07%) ⬇️
samples-tests-java8 29.74% <0.00%> (-0.05%) ⬇️
unit-tests-java11 59.28% <100.00%> (+0.16%) ⬆️
unit-tests-java17 58.76% <100.00%> (+0.12%) ⬆️
unit-tests-java21 58.79% <100.00%> (+0.18%) ⬆️
unit-tests-java25 58.72% <100.00%> (+0.16%) ⬆️
unit-tests-java8 59.30% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@juzi050
juzi050 force-pushed the fix/triple-rest-post-decoder-cleanup branch from cb5bce3 to 07ba80f Compare August 1, 2026 19:29
@juzi050
juzi050 force-pushed the fix/triple-rest-post-decoder-cleanup branch from 0da1ab2 to b931dc5 Compare August 2, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Triple REST does not clean up HttpPostRequestDecoder after a form request completes

2 participants