Skip to content

[fix][client] prevent garbage collection of token-supplier #412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 11, 2025
Merged

[fix][client] prevent garbage collection of token-supplier #412

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98a757a
Add persistent reference to Auth Object
klaatu01 May 6, 2025
9ab7dea
Merge branch 'apache:master' into master
klaatu01 May 6, 2025
ab8ba30
Update src/Client.cc
BewareMyPower May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ Client::Client(const Napi::CallbackInfo &info) : Napi::ObjectWrap<Client>(info)
if (clientConfig.Has(CFG_AUTH) && clientConfig.Get(CFG_AUTH).IsObject()) {
Napi::Object obj = clientConfig.Get(CFG_AUTH).ToObject();
if (obj.Has(CFG_AUTH_PROP) && obj.Get(CFG_AUTH_PROP).IsObject()) {
Authentication *auth = Authentication::Unwrap(obj.Get(CFG_AUTH_PROP).ToObject());
this->authRef_ = Napi::Persistent(obj.Get(CFG_AUTH_PROP).As<Napi::Object>());
this->authRef_.SuppressDestruct();
Copy link
Contributor

Choose a reason for hiding this comment

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

SuppressDestruct() suppresses the destruction of authRef_, which is held by the Client instance. Could you try removing this call?

This method is used here:

pulsar-client-node/src/Client.cc

Line 88 in b79ead0

constructor.SuppressDestruct();

However, constructor is a static variable, whose lifetime is the same with the whole program. But here authRef_'s lifetime is the same with the Client. Suppressing the destruction of authRef_ could cause memory leak.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't have time looking deeper for now. But I assume that when the Client destructs, the authRef_ will be destroyed as well unless you call SuppressDestruct().

The original issue happens just because auth is a local variable that is referenced by the client config's authentication field. However, since it's now a field of Client, whose lifetime should be longer than its config. We can now prevent the GC of auth.

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @RobertIndie Please help double confirm if there is anything wrong with my analysis above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's the case then perhaps removing that for the logger object will also stop that other open handle.

I am away this weekend, but will give this a go when I'm back.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your analysis. I think it's better to add a test to verify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I am on mobile just seen that you have done it and tests passed!

Authentication *auth = Authentication::Unwrap(this->authRef_.Value());
pulsar_client_configuration_set_auth(cClientConfig.get(), auth->GetCAuthentication());
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class Client : public Napi::ObjectWrap<Client> {
std::shared_ptr<pulsar_client_t> cClient;
std::shared_ptr<pulsar_client_configuration_t> cClientConfig;
pulsar_logger_level_t logLevel = pulsar_logger_level_t::pulsar_INFO;
Napi::ObjectReference authRef_;

Napi::Value CreateProducer(const Napi::CallbackInfo &info);
Napi::Value Subscribe(const Napi::CallbackInfo &info);
Expand Down
Loading