apache / apache/dubbo-go

[BUG] triple: server response attachments silently dropped (discarded AppendToOutgoingContext return)

Open
#3,511 0 comments 0 reactions 1 assignee Claimed by @Aias00 View on GitHub
☢️ Bug 3.3.3
Dominant language
Go
Stars
5k
Forks
1k
Avg merge
2d 8h
Merged PRs (30d)
31

Description

### Problem

The server-side unary handler calls `appendTripleOutgoingAttachments(ctx, res.Attachments())` to propagate provider-set response attachments to the client. However, `tri.AppendToOutgoingContext` follows gRPC metadata semantics: it **returns a new `context.Context`** (it does `ctx = context.WithValue(ctx, extraDataKey{}, extraData)` when the context has no pre-existing outgoing data). `appendTripleOutgoingAttachments` discards that return value, and the function does not reassign `ctx`.

The server handler context is never initialized with an outgoing context (`handler.go` only sets `handlerOutgoingKey`, not `extraDataKey`). Therefore the first `AppendToOutgoingContext` call takes the `!ok` branch, creates the outgoing header map on a *new* context that is immediately thrown away, and the subsequent `ExtractFromOutgoingContext(ctx)` in `handler.go` reads the *original* context → returns `nil` → `mergeHeaders(conn.ResponseTrailer(), nil)` writes nothing.

Result: provider response attachments returned via `res.Attachments()` are silently dropped and never reach the client trailer. No error is raised.

This mechanism was introduced by #2928 (which switched the server path from `triResp.Trailer().Set` to `AppendToOutgoingContext`). The discard appears to have been latent since then. It is distinct from #3445, which is about the consumer-side generic API for *reading* response trailers.

### Current behavior

`protocol/triple/server.go:557-560` (call site)
```go
res := invoker.Invoke(ctx, invo)
triResp := wrapTripleResponse(res.Result())
appendTripleOutgoingAttachments(ctx, res.Attachments()) // return value not captured
return triResp, res.Error()
```

`protocol/triple/server.go:638-649` (helper discards the returned context)
```go
func appendTripleOutgoingAttachments(ctx context.Context, attachments map[string]any) {
for k, v := range attachments {
switch val := v.(type) {
case string:
tri.AppendToOutgoingContext(ctx, k, val) // returned ctx discarded
case []string:
for _, item := range val {
tri.AppendToOutgoingContext(ctx, k, item) // returned ctx discarded
}
}
}
}
```

`protocol/triple/triple_protocol/header.go:176-194` (immutable return)
```go
func AppendToOutgoingContext(ctx context.Context, kv ...string) context.Context {
extraData, ok := ctx.Value(extraDataKey{}).(map[string]http.Header)
if !ok {
extraData = map[string]http.Header{}
ctx = context.WithValue(ctx, extraDataKey{}, extraData) // new context
}
...
return ctx
}
```

`protocol/triple/triple_protocol/handler.go:117` (server reads the original context → always nil)
```go
if data := ExtractFromOutgoingContext(ctx); data != nil {
mergeHeaders(conn.ResponseTrailer(), data)
}
```

### Expected behavior

1. Provider attachments set via `res.Attachments()` (string / `[]string`) must reach the response trailer and be readable by the client.
2. Non-string attachment values should not be silently ignored (current `switch` has no `default`).
3. Either `appendTripleOutgoingAttachments` returns the updated context and the caller uses it, or the server uses the working `tri.SetHeader`/`SetTrailer` (`handlerOutgoingKey → conn`) path consistently.

### Suggested approach

- Change `appendTripleOutgoingAttachments` to return `context.Context` (`ctx = tri.AppendToOutgoingContext(ctx, k, v)` in the loop) and update the call site: `ctx = appendTripleOutgoingAttachments(ctx, res.Attachments())`, then ensure the framework serializes `ExtractFromOutgoingContext(ctx)` into the response trailer (it already does at `handler.go:117`).
- Or bypass the outgoing-context mechanism entirely and write response attachments via `triResp.Trailer().Set(...)` (the pre-#2928 path), which does not depend on context immutability.
- Handle non-string attachment types explicitly (convert or warn).
- Add an end-to-end test: provider returns `res.Attachments()` → client reads trailer (`tri.FromIncomingContext` / `WithResponseTrailer`) and asserts the values.

### Acceptance criteria

- [ ] Provider `res.Attachments()` string/`[]string` values are received by the client response trailer.
- [ ] Non-string attachment types are no longer silently dropped (converted or warned).
- [ ] End-to-end test covers server→client attachment propagation for unary calls.
- [ ] Existing triple header/trailer tests remain green.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.