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

Issue 14583: [python client]: Improve garbage collection of producer/consumer/reader objects #14585

Conversation

zbentley
Copy link
Contributor

@zbentley zbentley commented Mar 7, 2022

Fixes apache/pulsar-client-python#42

Motivation

The Python client bindings make it very easy to leak Pulsar client objects. Those leaks manifest in segfaults and program crashes (e.g. #14582).

Modifications

Adds an abstract superclass of all stateful python client objects (clients, consumers, readers, producers) which coordinates garbage collection of those objects, according to these principles:

  • Disposing of an object should close its underlying resources.
  • If the only reference a program holds is to a "child" object (one descended from a client) and that child is disposed of, the parent client that created it should also then be disconnected and destroyed.

Verifying this change

This change added tests and can be verified as follows:

Documentation

  • no-need-doc
    I do not believe that this PR affects external API contracts or behavior.

@github-actions
Copy link

github-actions bot commented Mar 7, 2022

@zbentley:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

github-actions bot commented Mar 7, 2022

@zbentley:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 7, 2022
@merlimat merlimat added component/python type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Mar 7, 2022
@merlimat merlimat added this to the 2.11.0 milestone Mar 7, 2022
@merlimat merlimat requested a review from BewareMyPower March 7, 2022 20:33
@BewareMyPower
Copy link
Contributor

Could you fix the license check?

@github-actions
Copy link

github-actions bot commented Apr 8, 2022

The pr had no activity for 30 days, mark with Stale label.

@zbentley
Copy link
Contributor Author

Fixing license check and tests this week or next, sorry for delay.

@BewareMyPower
Copy link
Contributor

Traceback (most recent call last):
  File "pulsar_test.py", line 25, in <module>
    import pulsar
  File "/usr/local/lib/python2.7/dist-packages/pulsar/__init__.py", line 103, in <module>
    from abc import ABC
ImportError: cannot import name ABC

It looks like the ABC package doesn't exist in Python2, while the CI still uses Python2.

@merlimat
Copy link
Contributor

It looks like the ABC package doesn't exist in Python2, while the CI still uses Python2.

I think the py2 support at this time is out of any reasonable time :)

I just submitted proposal to drop Py2. #15185. We should be able to do that in few days.

@zbentley
Copy link
Contributor Author

Unsure why the two most recent commits haven't finished their build. Is there an issue with CI? Or a problem with this PR?

@BewareMyPower
Copy link
Contributor

You can see error logs in https://github.com/apache/pulsar/runs/6155320188?check_suite_focus=true.

Exception AttributeError: "'Client' object has no attribute '_handle'" in <bound method Client.__del__ of <pulsar.Client object at 0x7f0582d10ed0>> ignored

Debugging for the Python2 code is somehow troublesome. I usually build the Python2 wheels in a CentOS 7 docker with docker-build.sh and run tests inside the container.

If you are not in a hurry to merge this PR, maybe it's better to wait until PIP-155 is done. After that, you can restore this PR to the Python3 compatible code.

@zbentley
Copy link
Contributor Author

@BewareMyPower there's no hurry. I'll fix up this code regardless since it needed a few tweaks. The build failures weren't due to that AttributeError, though (destruction time errors are ugly, but don't cause tests to fail with exceptions); they seemed to be due to the BROKER_CONNECTION, FUNCTION, and FLAKY builds.

@github-actions
Copy link

github-actions bot commented Jun 5, 2022

The pr had no activity for 30 days, mark with Stale label.

@BewareMyPower
Copy link
Contributor

@zbentley Could you resolve the conflicts in master branch?

P.S. This PR can only be cherry-picked into branch-2.11 because Pulsar drops the Python2 support since 2.11.0

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

@zbentley Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@github-actions github-actions bot removed the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022
@tisonkun
Copy link
Member

Closed as the development of the Python client has been permanently moved to https://github.com/apache/pulsar-client-python.

@tisonkun tisonkun closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-label-missing release/2.11.1 Stale type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
5 participants