laskoviymishka commented on code in PR #1606:
URL: https://github.com/apache/iceberg-go/pull/1606#discussion_r3681139994
##########
catalog/rest/rest_internal_test.go:
##########
@@ -1696,6 +1696,15 @@ func (t *closeTrackingTransport) CloseIdleConnections() {
t.closed.Store(true)
}
+func TestCatalogCloseReleasesOwnedSessionOnce(t *testing.T) {
Review Comment:
This test doesn't actually exercise the fix.
It constructs a `Catalog` literal with `closeSession` already populated, so
all it proves is that the `sync.Once` wrapper fires the callback once. The one
line that fixes the leak — `r.cl, r.closeSession, err = r.createSession(ctx,
ops)` in `init` — is entirely outside its scope. Revert that back to `r.cl, _,
err = ...` and this test still passes green, which is exactly the regression
we'd want it to catch.
I'd add an integration-level test that builds a real catalog through
`NewCatalog`/`newCatalogFromProps` with an internally-owned transport, calls
`cat.Close()`, and asserts that transport's `CloseIdleConnections` ran —
`closeTrackingTransport` is already right there. And while we're in here, it'd
be worth mirroring `TestFetchConfigDoesNotCloseCustomTransport` for `Close()`:
build a catalog `WithCustomTransport(base)`, close it, assert `base.closed`
stays false, so the caller-ownership boundary is pinned too. Right now that
invariant is only documented, not tested.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -1696,6 +1696,15 @@ func (t *closeTrackingTransport) CloseIdleConnections() {
t.closed.Store(true)
}
+func TestCatalogCloseReleasesOwnedSessionOnce(t *testing.T) {
+ var calls atomic.Int32
Review Comment:
Every other top-level test in this file calls `t.Parallel()` and there's no
shared state forcing this one to be serial — I'd add it as the first line to
stay consistent (and keep `paralleltest` happy if it's enabled).
##########
catalog/rest/rest.go:
##########
@@ -1066,11 +1071,18 @@ func (r *Catalog) fetchConfig(ctx context.Context, opts
*options) (*options, err
func (r *Catalog) Name() string { return r.name }
func (r *Catalog) CatalogType() catalog.Type { return catalog.REST }
-// Close releases the catalog's metrics reporter. The REST catalog does not own
-// the lifetime of the HTTP client it was configured with, so only the reporter
-// is released. Callers holding a [catalog.Catalog] can reach this via a
-// [catalog.Closer] type assertion.
-func (r *Catalog) Close() error { return r.reporter.Close() }
+// Close releases transports created by the catalog and its metrics reporter.
+// Caller-provided transports remain caller-owned. Callers holding a
+// [catalog.Catalog] can reach this via a [catalog.Closer] type assertion.
+func (r *Catalog) Close() error {
+ r.closeSessionOnce.Do(func() {
+ if r.closeSession != nil {
+ r.closeSession()
+ }
+ })
+
+ return r.reporter.Close()
Review Comment:
`reporter.Close()` sits outside the `Once`, so it fires on every `Close()`
call while the session cleanup fires once.
It's not a live bug — `CachedReporter.Close()` is idempotent today (it nils
itself under a mutex, so the second call is a no-op). But the new test
advertises `Close()` as safe to call twice, and that guarantee currently leans
on an undocumented property of the concrete reporter; a future non-idempotent
or wrapped reporter breaks double-close callers quietly.
I'd pull `reporter.Close()` inside the `Once` so the whole method is
idempotent as one unit, and capture the error:
```go
func (r *Catalog) Close() error {
var err error
r.closeSessionOnce.Do(func() {
if r.closeSession != nil {
r.closeSession()
}
err = r.reporter.Close()
})
return err
}
```
The godoc could also mention `Close` is safe to call more than once, since
that's now the intent (`closeSessionOnce`). wdyt?
--
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]
