SLoeuillet commented on code in PR #2079:
URL: https://github.com/apache/iceberg-go/pull/2079#discussion_r4191872057
##########
catalog/rest/vended_creds.go:
##########
@@ -171,33 +176,62 @@ func (v *vendedCredentialRefresher) loadFS(ctx
context.Context) (iceio.IO, error
maps.Copy(config, freshCreds)
}
+ // The IO is cached and shared by every later caller, so it must not
+ // inherit this caller's cancellation: a filesystem may keep the
context it
+ // is opened with (blobfs does, so every gocloud backend), and a cached
IO
+ // built on a per-operation context would fail every later operation
with
+ // context.Canceled once that context is done. The refresher owns the
IO's
+ // lifetime instead, through ioCancel. fetchCreds above still honours
ctx.
+ //
+ // WithoutCancel keeps ctx's values, and the IO depends on them: the S3
+ // factory takes its base AWS config from utils.GetAwsConfig(ctx), so do
+ // not replace this with context.Background(). The flip side is that the
+ // first caller's request-scoped values stay attached to the shared IO.
+ ioCtx, ioCancel := context.WithCancel(context.WithoutCancel(ctx))
+
+ var (
+ newIO iceio.IO
+ expiresAt time.Time
+ issuedAt = v.issuedAt
+ )
+
if len(v.credentials) > 0 {
- prefixIO := newPrefixScopedIO(ctx, v.props, v.credentials)
+ prefixIO := newPrefixScopedIO(ioCtx, v.props, v.credentials)
prefixIO.nowFunc = v.now
- v.cachedIO = prefixIO
+ newIO = prefixIO
// Expiry is enforced by prefixScopedIO against only the
credential
// selected for each object location.
- v.expiresAt = time.Time{}
+ } else {
+ loaded, err := iceio.LoadFS(ioCtx, config, v.location)
+ if err != nil {
+ ioCancel()
- return v.cachedIO, nil
- }
+ if v.cachedIO == nil {
+ return nil, err
+ }
- newIO, err := iceio.LoadFS(ctx, config, v.location)
- if err != nil {
- if v.cachedIO == nil {
- return nil, err
+ return nil, fmt.Errorf("load filesystem with refreshed
credentials for %s: %w", v.location, err)
}
- return nil, fmt.Errorf("load filesystem with refreshed
credentials for %s: %w", v.location, err)
+ newIO = loaded
+ expiresAt = v.expiresAtFromConfig(config)
+ issuedAt = v.now()
}
- v.cachedIO = newIO
- v.expiresAt = v.expiresAtFromConfig(config)
- v.issuedAt = v.now()
+ v.replaceIO(newIO, ioCancel)
+ v.expiresAt = expiresAt
+ v.issuedAt = issuedAt
return v.cachedIO, nil
}
+// replaceIO installs io as the cached IO. The superseded IO is left open:
+// callers that loaded it earlier may still be using it, and its credentials
+// remain valid until their own expiry.
+func (v *vendedCredentialRefresher) replaceIO(io iceio.IO, cancel
context.CancelFunc) {
+ v.cachedIO, v.ioCancel = io, cancel
Review Comment:
Done in 2883aef: the `replaceIO` doc now says the superseded cancel is
dropped on purpose, and why.
--
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]
