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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
