Repository navigation
fix(sdk): give each invocation independently owned header values (#7771) - #7786
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Approving. I reproduced the root cause independently and the fix lands at the right layer.
Why the defect is real
from() was not the only place that shared collections: the per-method template handed to ShenyuClientMethodHandler is populated by SpringMvcContract, and every static header there is an immutable singleton:
SpringMvcContract:111 requestTemplate.getHeaders().put(ACCEPT, Collections.singleton(clientAccepts));
SpringMvcContract:119 requestTemplate.getHeaders().put(CONTENT_TYPE, Collections.singleton(clientProduces));
SpringMvcContract:123-131 parseHeaders -> Collections.singleton(resolve(...))
ShenyuClientMethodHandler#targetProcessor:87-89 copies that template with from() and then asks request() for the per-call request, and request() (RequestTemplate:89) only copies the outer map — so the value collection stayed shared with the long-lived template. Consequence on master, exactly as described: RequestHeaderParameterProcessor's single-value branch (headerColl.add(...)) and its map branch (old.add(...)) hit UnsupportedOperationException when a static @Headers/produces/consumes header collides with a dynamic one, and @CookieValue accumulated template-owned cookies across invocations.
Why the fix is at the right level
from()per-entry copy makes the per-invocation template a real snapshot;request()'s shallow outer-map copy stays a cheap optimisation because the values it hands out are now owned by that snapshot.- Copying again before appending in both processors is still needed (the request's values are the snapshot's values, and the snapshot is reused if
request()is called twice), so both halves of the change are justified rather than redundant. CookieValueParameterProcessorreading the per-call map instead of the template is a behaviour improvement, not just a refactor: two@CookieValueparameters on one method used to append the second value into a collection that already contained it. A test with two cookie parameters would pin that down.
Non-blocking
new ArrayList<>(value)throws NPE if a template ever holds anullvalue entry. No writer in the repo does that today, but aObjects.requireNonNull/skip would make the copy total.- The "Notes" section says a user
ShenyuSdkRequestInterceptorcan still hit an immutable value list. For the standard path that is no longer true: afterfrom()every value in the per-call request is anArrayList, sorequest.getHeaders().get("X").add(...)from an interceptor is safe too. Suggest updating the description (or adding a test) so the next reader does not assume the hole is still open. - Worth documenting the ownership contract on
request()as well: its value collections belong to the template it is invoked on and must not be mutated by callers. That is the invariant the two processors now rely on.
CI on 78a58a9 is green (pr_build, it, sdk-http/sdk-apache-dubbo, e2e).
Fixes #7771.
Summary
RequestTemplate.from()shares header value collections with the template it copies from. SincetargetProcessorrunsfrom→request()→ parameter processors against a long-lived per-method template, dynamic header/cookie values mutate collections the template still owns: a staticCollections.singletonheader paired with a dynamic value of the same name throwsUnsupportedOperationExceptionbefore any transport call, and dynamic cookies accumulate across invocations.Modifications
RequestTemplate.from()copies header value collections per entry (new ArrayList<>), giving each invocation independently owned values.RequestHeaderParameterProcessorcopies an existing collection before appending (both the single-value and map branches).CookieValueParameterProcessorbuilds the per-callCookieentry from the invocation's own header map with a defensive copy, and no longer reads or mutates the shared template; its previous compute path could also double-append template values.Verifying this change
New tests:
RequestTemplateTest#testFromCopiesHeaderValues(mutating the copy leaves the original untouched),RequestHeaderParameterProcessorTest#processArgumentOnStaticSingletonHeaderTest(static singleton + dynamic same-name header merges, template value unchanged) andCookieValueParameterProcessorTest#processArgumentDoesNotAccumulateAcrossCalls(two invocations stay isolated, template never grows).Red-green: with the deep copy and the cookie read-point change reverted, the singleton and accumulation cases fail with
UnsupportedOperationException; with the fix all pass (core 4/4, spring 6/6 + 6/6).Targeted tests pass:
core 4/4, spring 6/6 + 6/6 green.
Checkstyle passes:
clean.
Notes
Transport-layer consumers (OkHttp/Http clients, load-balance data) only read headers, so per-entry copying has no behavioural downside.
request()'s shallow outer-map copy stays: afterfrom(), the value collections are already independently owned. A userShenyuSdkRequestInterceptorthat appends to an existing collection could still hit an immutable value list of its own making — same family of issue, left untouched here since the interceptor map is already per-request at the outer level.