Skip to content

grpclb/state, attributes: implement Stringer for grpclb keyType and document <%p> rationale - #9383

Closed
pefoley2 wants to merge 1 commit into
grpc:masterfrom
pefoley2:fix-attributes-formatting
Closed

grpclb/state, attributes: implement Stringer for grpclb keyType and document <%p> rationale#9383
pefoley2 wants to merge 1 commit into
grpc:masterfrom
pefoley2:fix-attributes-formatting

Conversation

@pefoley2

@pefoley2 pefoley2 commented Aug 28, 2026

Copy link
Copy Markdown

Description

  1. balancer/grpclb/state: Implements fmt.Stringer on keyType (func (k keyType) String() string { return string(k) }). This ensures that grpclb state attributes format cleanly as "grpc.grpclb.state" instead of falling back to <%!p(state.keyType=grpc.grpclb.state)> (due to state.keyType being a distinct named type that fails direct x.(string) interface assertions).
  2. attributes: Adds documentation to str() explaining why <%p> is used for pointer/arbitrary types and why %v / %#v must be avoided to prevent unsynchronized field reads and data races on mutex-protected structs (as documented in attributes: avoid the use of %#v formatting verb #6664).

Tests

  • Added TestStateAttributesString in balancer/grpclb/state/state_test.go.
  • Ran unit tests across attributes and balancer/grpclb/state packages.

Avoid formatting errors (%!p(...)) on value types and pointers when
formatting attributes, while preserving full type and value information.
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.58%. Comparing base (6d697e4) to head (0ab28c9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9383      +/-   ##
==========================================
+ Coverage   87.54%   87.58%   +0.03%     
==========================================
  Files         429      429              
  Lines       30622    30624       +2     
==========================================
+ Hits        26807    26821      +14     
+ Misses       3814     3802      -12     
  Partials        1        1              
Files with missing lines Coverage Δ
attributes/attributes.go 100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eshitachandwani

Copy link
Copy Markdown
Member

Thanks for working on this PR! However, we cannot accept this change because using %v as a fallback re-introduces a data race when formatting attributes.

This was previously investigated and fixed in #6664.

At the time, Attributes.String() was formatting values using the %#v verb. When pointers to structs (such as *MyStruct) are stored as keys or values in attributes.Attributes, those structs often contain internal state protected by a sync.Mutex. Because Go's standard library fmt package uses reflection to inspect struct fields without awareness of synchronization primitives, formatting them reads their fields without acquiring the struct's mutex. If another goroutine writes to those fields concurrently, this triggers a data race under the Go race detector (go test -race), causing widespread test flakiness.

In PR #6664, we intentionally switched the fallback to <%p> because %p only formats the pointer's memory address without dereferencing it or reading underlying struct fields.

Changing the fallback to %T=%v causes the exact same issue:

  • When x is a pointer to a struct without a fmt.Stringer implementation, %v dereferences the pointer and iterates over its struct fields (printing &{...}).
  • Reading these fields without holding the struct's mutex creates an unsynchronized read against concurrent writes.
  • We can see this exact race failure occurring in CI on this PR under the race detector: CI Run 33181649126.

While the <%!p(...)> formatting for primitive scalar types (like int or bool) is not ideal, avoiding data races and test flakiness on mutable pointer types takes precedence.

So, closing this PR.

@pefoley2 pefoley2 changed the title attributes: format non-Stringer, non-string types as %T=%v grpclb/state, attributes: implement Stringer for grpclb keyType and document <%p> rationale Aug 31, 2026
@pefoley2

Copy link
Copy Markdown
Author

Thanks for the feedback and context around #6664! I have reworked the changes:

  1. attributes: Kept the <%p> fallback in attributes.go and added a comment on str() explaining why <%p> is used and why %v/%#v cannot be used on struct pointers (to prevent data races on mutex-protected fields as investigated in attributes: avoid the use of %#v formatting verb #6664).
  2. balancer/grpclb/state: Implemented fmt.Stringer on keyType in state.go (func (k keyType) String() string { return string(k) }). This fixes the specific <%!p(state.keyType=grpc.grpclb.state)> issue: because keyType is a defined named type, direct x.(string) interface assertion evaluated to false; implementing fmt.Stringer allows it to format cleanly as "grpc.grpclb.state".

Could you please reopen this PR (or let me know if you would prefer a fresh PR for this)?

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.

2 participants