Skip to content

Commit e9a68b7

Browse files
authored
fix(rest): avoid default catalog reference cycle (#764)
Store the cached default catalog view as a weak_ptr so AsCatalog() can reuse a live view without keeping it alive from the RestCatalog root.
1 parent 255657b commit e9a68b7

3 files changed

Lines changed: 32 additions & 5 deletions

File tree

src/iceberg/catalog/rest/rest_catalog.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -487,11 +487,13 @@ RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> f
487487
std::string_view RestCatalog::name() const { return name_; }
488488

489489
Result<std::shared_ptr<Catalog>> RestCatalog::AsCatalog() {
490-
if (!default_catalog_) {
491-
default_catalog_ =
492-
std::make_shared<ContextCatalog>(shared_from_this(), default_context_);
490+
if (auto catalog = default_catalog_.lock()) {
491+
return catalog;
493492
}
494-
return default_catalog_;
493+
494+
auto catalog = std::make_shared<ContextCatalog>(shared_from_this(), default_context_);
495+
default_catalog_ = catalog;
496+
return catalog;
495497
}
496498

497499
Result<std::shared_ptr<Catalog>> RestCatalog::WithContext(SessionContext context) {

src/iceberg/catalog/rest/rest_catalog.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ class ICEBERG_REST_EXPORT RestCatalog final
181181
std::shared_ptr<auth::AuthSession> catalog_session_;
182182
SnapshotMode snapshot_mode_;
183183
SessionContext default_context_;
184-
std::shared_ptr<Catalog> default_catalog_;
184+
std::weak_ptr<Catalog> default_catalog_;
185185
};
186186

187187
} // namespace iceberg::rest

src/iceberg/test/rest_catalog_integration_test.cc

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,31 @@ TEST_F(RestCatalogIntegrationTest, MakeCatalogSuccess) {
203203
EXPECT_THAT(root->WithContext(SessionContext{}), IsError(ErrorKind::kInvalidArgument));
204204
}
205205

206+
TEST_F(RestCatalogIntegrationTest, DefaultCatalogCacheDoesNotKeepRootAlive) {
207+
std::weak_ptr<RestCatalog> weak_root;
208+
std::weak_ptr<Catalog> weak_catalog;
209+
210+
{
211+
auto config = RestCatalogProperties::default_properties();
212+
config.Set(RestCatalogProperties::kUri, CatalogUri())
213+
.Set(RestCatalogProperties::kName, std::string(kCatalogName))
214+
.Set(RestCatalogProperties::kWarehouse, std::string(kWarehouseName));
215+
config.mutable_configs()[std::string(RestCatalogProperties::kIOImpl.key())] =
216+
std::string(kStdFileIOImpl);
217+
218+
ICEBERG_UNWRAP_OR_FAIL(auto root, RestCatalog::Make(config));
219+
ICEBERG_UNWRAP_OR_FAIL(auto catalog, root->AsCatalog());
220+
ICEBERG_UNWRAP_OR_FAIL(auto same_catalog, root->AsCatalog());
221+
222+
weak_root = root;
223+
weak_catalog = catalog;
224+
EXPECT_EQ(catalog, same_catalog);
225+
}
226+
227+
EXPECT_TRUE(weak_catalog.expired());
228+
EXPECT_TRUE(weak_root.expired());
229+
}
230+
206231
TEST_F(RestCatalogIntegrationTest, FetchServerConfigDirect) {
207232
HttpClient client({});
208233
auto noop_session = auth::AuthSession::MakeDefault({});

0 commit comments

Comments
 (0)