Skip to content
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.

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

[fix] [client] Fix resource leak in Pulsar Client since HttpLookupService doesn't get closed #22858

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

poorbarcode
Copy link
Contributor

Motivation

#21356 added a resource map urlLookupMap in PulsarClientImpl.class, but did not release the lookup services when closing PulsarClient.

Modifications

Release urlLookupMap when closing PulsarClient

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug release/3.3.1 labels Jun 6, 2024
@poorbarcode poorbarcode added this to the 3.4.0 milestone Jun 6, 2024
@poorbarcode poorbarcode self-assigned this Jun 6, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 6, 2024
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

I don't think it's actual a memory leak. This PR only clears the map after closeAsync is called. However, I could hardly think of a case that a PulsarClient is still used after closeAsync. When the PulsarClientImpl object is GC'ed, this map will be GC'ed as well.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari changed the title [fix] [client] Fix memory leak when using API pulsarClient.getLookup(url) [fix] [client] Fix resource leak when using API pulsarClient.getLookup(url) Jun 6, 2024
@lhotari
Copy link
Member

lhotari commented Jun 6, 2024

I don't think it's actual a memory leak.

@BewareMyPower I agree. I renamed "memory leak" to "resource leak" in the title. /cc @poorbarcode

Threads get leaked since the underlying HttpClient of the HttpLookupService doesn't get closed.

@lhotari lhotari changed the title [fix] [client] Fix resource leak when using API pulsarClient.getLookup(url) [fix] [client] Fix resource leak in Pulsar Client since HttpLookupService doesn't get closed Jun 6, 2024
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@poorbarcode poorbarcode requested review from BewareMyPower and removed request for BewareMyPower June 11, 2024 02:28
@BewareMyPower BewareMyPower merged commit bc3dc77 into apache:master Jun 18, 2024
64 of 66 checks passed
@poorbarcode poorbarcode deleted the fix/leak_urlLookupMap branch June 19, 2024 07:40
lhotari pushed a commit that referenced this pull request Jun 19, 2024
…vice doesn't get closed (#22858)

(cherry picked from commit bc3dc77)
lhotari pushed a commit that referenced this pull request Jun 19, 2024
…vice doesn't get closed (#22858)

(cherry picked from commit bc3dc77)
lhotari pushed a commit that referenced this pull request Jun 19, 2024
…vice doesn't get closed (#22858)

(cherry picked from commit bc3dc77)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 21, 2024
…vice doesn't get closed (apache#22858)

(cherry picked from commit bc3dc77)
(cherry picked from commit e9264a9)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 24, 2024
…vice doesn't get closed (apache#22858)

(cherry picked from commit bc3dc77)
(cherry picked from commit e9264a9)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 25, 2024
…vice doesn't get closed (apache#22858)

(cherry picked from commit bc3dc77)
(cherry picked from commit e9264a9)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
…vice doesn't get closed (apache#22858)

(cherry picked from commit bc3dc77)
(cherry picked from commit e9264a9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants