You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ilidemi (via GitHub)" <gi...@apache.org> on 2026/10/22 01:12:34 UTC
[PR] fix(catalog/rest): make OAuth token refresh context-aware and singleflight [iceberg-go]
ilidemi opened a new pull request, #2265:
URL: https://github.com/apache/iceberg-go/pull/2265
Fixes #2264.
## Summary
```diff
AuthHeaderWithContext(ctx)
if ctx already done → return ctx.Err()
- tokenSource.Token() // blocks until the refresh client's Timeout
+ select
+ ctx done → return ctx.Err()
+ shared refresh → header // one Token() in flight per manager
```
A `singleflight.Group` on `Oauth2AuthManager` dedupes `Token()` across concurrent callers. A caller that stops waiting leaves nothing queued behind the shared call, so a stalled endpoint sees one request however many callers gave up on it. The refresh keeps running on the token-refresh client, still bounded by its `Timeout`, and later callers reuse its result. `AuthHeader` goes through the same group so both entry points share one refresh. The error mapping in `authHeader` is unchanged; it now receives singleflight's untyped result.
Completes the context threading suggested earlier in the #1655 review (https://github.com/apache/iceberg-go/pull/1655#discussion_r3737958963). That review warned that wrapping the context-free call in a goroutine only relocates the leak; the singleflight is the answer, since there is at most one such goroutine per manager and it terminates with the client `Timeout`.
## Evidence
- **Before:** with the select removed, `TestOauth2AuthManager_AuthHeaderWithContext_CallerDeadline` hangs until `go test` kills it (`panic: test timed out after 15s`); with a per-caller singleflight key, `TestOauth2AuthManager_AuthHeaderWithContext_SharesRefresh` fails with `waiting callers must share one refresh`, expected 1 `Token()` call, got 5.
**After:** `go test -count=3 -race -run Oauth2AuthManager ./catalog/rest/` → `ok 2.3s`; full `./catalog/rest/` under `-race` → `ok 5.7s`; `golangci-lint` → 0 issues.
## Merge Danger
**Door:** two-way
Revert restores the previous synchronous behavior; no persisted state or wire format involved.
**Blast Radius:** REST catalog OAuth callers
Two observable changes. `AuthHeaderWithContext` can now return `ctx.Err()` for a caller whose context ends during a refresh, where it previously blocked and then returned the refresh error. Callers joining an in-flight refresh share its outcome, including its error, instead of each running their own attempt. Static-token and cached-token paths are unchanged apart from passing through singleflight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest)!: make OAuth token refresh context-aware and singleflight [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #2265:
URL: https://github.com/apache/iceberg-go/pull/2265#issuecomment-6361655111
@ilidemi welcome to contributors club!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest)!: make OAuth token refresh context-aware and singleflight [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #2265:
URL: https://github.com/apache/iceberg-go/pull/2265#discussion_r4353117881
##########
catalog/rest/auth_test.go:
##########
@@ -117,3 +120,73 @@ func TestOauth2AuthManager_AuthHeader_FetchToken_ErrorResponse(t *testing.T) {
assert.Contains(t, err.Error(), "invalid_client")
assert.Contains(t, err.Error(), "Invalid client credentials")
}
+
+func TestOauth2AuthManager_AuthHeaderWithContext_CallerDeadline(t *testing.T) {
+ mux := http.NewServeMux()
+ server := httptest.NewServer(mux)
+ defer server.Close()
+
+ // The token endpoint stalls until the test ends. Nothing cancels the refresh
+ // request itself, so the release must come before server.Close waits on it.
+ release := make(chan struct{})
+ defer close(release)
+ mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
+ <-release
+ })
+
+ cfg := &clientcredentials.Config{
+ ClientID: "client",
+ ClientSecret: "secret",
+ TokenURL: server.URL + "/oauth/token",
+ AuthStyle: oauth2.AuthStyleInParams,
+ }
+
+ tokenCtx := context.WithValue(context.Background(), oauth2.HTTPClient, server.Client())
+ manager := &Oauth2AuthManager{
+ tokenSource: cfg.TokenSource(tokenCtx),
+ }
+
+ ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+ defer cancel()
+ _, _, err := manager.AuthHeaderWithContext(ctx)
+
+ require.ErrorIs(t, err, context.DeadlineExceeded)
+}
+
+// blockingTokenSource counts Token calls and holds each until released.
+type blockingTokenSource struct {
+ calls atomic.Int32
+ release chan struct{}
+}
+
+func (s *blockingTokenSource) Token() (*oauth2.Token, error) {
+ s.calls.Add(1)
+ <-s.release
+
+ return &oauth2.Token{AccessToken: "shared_token", TokenType: "Bearer"}, nil
+}
+
+func TestOauth2AuthManager_AuthHeaderWithContext_SharesRefresh(t *testing.T) {
+ source := &blockingTokenSource{release: make(chan struct{})}
+ manager := &Oauth2AuthManager{tokenSource: source}
+
+ // Concurrent callers give up before the refresh completes.
+ var callers sync.WaitGroup
+ for range 5 {
+ callers.Go(func() {
+ ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+ defer cancel()
+ _, _, err := manager.AuthHeaderWithContext(ctx)
+ assert.ErrorIs(t, err, context.DeadlineExceeded)
+ })
+ }
+ callers.Wait()
+ assert.EqualValues(t, 1, source.calls.Load(), "waiting callers must share one refresh")
+
+ // The shared refresh completes in the background and serves later callers.
+ close(source.release)
+ key, value, err := manager.AuthHeader()
Review Comment:
Should the tail also check that `source.calls` is still 1 after `AuthHeader()`? As I read it, `release` is closed first, so if the background call has already finished and dropped the key, `Do` runs a fresh `Token()` that no longer blocks, and this passes either way. With the raw source that check would depend on ordering, so wrapping it in `oauth2.ReuseTokenSource`, as `clientcredentials` does, may be what pins it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest)!: make OAuth token refresh context-aware and singleflight [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #2265:
URL: https://github.com/apache/iceberg-go/pull/2265
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org