Skip to content

fix(rest): close internally owned transports - #1606

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/rest-close-transport
Open

fix(rest): close internally owned transports#1606
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/rest-close-transport

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • retain the cleanup callback for the final REST session
  • close catalog-owned HTTP and OAuth transports from Catalog.Close
  • keep caller-provided transports caller-owned
  • make session cleanup idempotent

Why

createSession builds cleanup callbacks for internally allocated transports, but initialization discarded the callback for the final long-lived session. Closing a catalog therefore left its idle connection pools alive.

Testing

  • go test ./catalog/rest

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 29, 2026 21:40

@laskoviymishka laskoviymishka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on the leak — throwing away createSession's cleanup with _ in init meant the long-lived session's internally-owned transports never got drained, and routing that cleanup through Close() behind a sync.Once (while leaving caller-provided transports alone) is exactly the right shape.

Almost there. One blocking thing before merge: the new test doesn't actually exercise the fix. It builds a Catalog literal with closeSession already set, so it only proves the sync.Once wrapper fires once — the line that fixes the bug (r.cl, r.closeSession, err = r.createSession(...)) is outside its scope, and reverting that line keeps the test green. For a regression test on a resource leak, I'd want it to fail if the wiring regresses, so I'd build a real catalog through the constructor with an internally-owned transport and assert CloseIdleConnections actually ran.

A couple of smaller things I'd want alongside that:

  • reporter.Close() sits outside the Once, so it fires on every call. Not a bug today since CachedReporter is idempotent, but I'd fold it into the Once so the whole method is idempotent as one unit, and note that in the godoc.
  • The catalog.Closer godoc in catalog/catalog.go still calls a stateful HTTP-backed closer a "future" thing — this PR makes it present, so that line is now stale.

Once the test actually pins the fix, happy to take another pass and approve.

t.closed.Store(true)
}

func TestCatalogCloseReleasesOwnedSessionOnce(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread catalog/rest/rest.go Outdated
}
})

return r.reporter.Close()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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?

Comment thread catalog/rest/rest_internal_test.go Outdated
}

func TestCatalogCloseReleasesOwnedSessionOnce(t *testing.T) {
var calls atomic.Int32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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