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 memory leak in msg.headers() #458

Merged
merged 1 commit into from Oct 10, 2018
Merged

Fix memory leak in msg.headers() #458

merged 1 commit into from Oct 10, 2018

Conversation

edenhill
Copy link
Contributor

@edenhill edenhill commented Sep 28, 2018

Moved interceptor inclusion work to own branch

@edenhill edenhill force-pushed the hdrsleak branch 3 times, most recently from 17845b1 to 1488fb2 Compare October 3, 2018 13:07
@edenhill edenhill changed the title Fix memory leak in msg.headers() Fix memory leak in msg.headers() and include monitoring-interceptors in binary wheels Oct 3, 2018
Copy link
Contributor

@rnpridgeon rnpridgeon left a comment

Choose a reason for hiding this comment

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

I'd add a quick update to the RELEASE.md to ensure we don't forget to keep the CP version up to date.

tools/install-interceptors.sh Outdated Show resolved Hide resolved
@edenhill
Copy link
Contributor Author

edenhill commented Oct 3, 2018

Will merge as soon as these packages are verified.

@edenhill
Copy link
Contributor Author

edenhill commented Oct 5, 2018

@rnpridgeon Might want to re-review this since there's been changes, thanks

@rnpridgeon
Copy link
Contributor

Added an additional step to validate interceptor packaging in the wheels. I did not add this as an after_* lifecycle as that would not fail the overall build job. Locally python was unable to locate the interceptor in its desired location using the v0.11.6-test6.

.travis.yml Outdated
@@ -66,7 +66,8 @@ script:
- if [[ -z $CIBW_BEFORE_BUILD ]]; then py.test -v --timeout 20 --ignore=tmp-build --import-mode append ; fi
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then cibuildwheel --output-dir wheelhouse1 && tools/fixup-wheels.sh wheelhouse1 wheelhouse ; fi
- if [[ -n $TRAVIS_TAG && $TRAVIS_OS_NAME == linux && -n $CIBW_BEFORE_BUILD ]]; then tools/test-manylinux.sh ; fi

- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka;\
py.test -v --timeout 20 --ignore=tmp-build --import-mode append; tests/test_misc.py::test_unordered_dict; fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... append; tests/test_misc...

Should there really be a ; in there?

Also, the tab indent is very red in the diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should probably use && instead of ; between pip install and py.test.
Another thing to note is that it is error prone to run the tests from within the confluent-kafka-python source tree since import confluent_kafka may pick up the local directory, better to cd out to the parent directory.

.travis.yml Outdated
@@ -66,8 +66,7 @@ script:
- if [[ -z $CIBW_BEFORE_BUILD ]]; then py.test -v --timeout 20 --ignore=tmp-build --import-mode append ; fi
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then cibuildwheel --output-dir wheelhouse1 && tools/fixup-wheels.sh wheelhouse1 wheelhouse ; fi
- if [[ -n $TRAVIS_TAG && $TRAVIS_OS_NAME == linux && -n $CIBW_BEFORE_BUILD ]]; then tools/test-manylinux.sh ; fi
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka;\
py.test -v --timeout 20 --ignore=tmp-build --import-mode append; tests/test_misc.py::test_unordered_dict; fi
- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka && cd .. && pytest -v --timeout 20 --ignore=tmp-build --import-mode append confluent-kafka-python/tests/test_misc.py::test_unordered_dict; fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put the cd and the command in a subshell, or cd .. back out, otherwise any sub-sequent commands will fail because the directory changed:

- if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then pip install --no-index -f wheelhouse/ confluent-kafka && (cd .. && pytest -v --timeout 20 --ignore=tmp-build --import-mode append confluent-kafka-python/tests/test_misc.py::test_unordered_dict); fi

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I guess there are no guarantees this will always be the last script executed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it will help the next poor chap that adds a command here.

@edenhill edenhill changed the title Fix memory leak in msg.headers() and include monitoring-interceptors in binary wheels Fix memory leak in msg.headers() Oct 10, 2018
@edenhill edenhill merged commit 4e1f5c9 into master Oct 10, 2018
@edenhill edenhill deleted the hdrsleak branch October 10, 2018 06:59
@oranb oranb mentioned this pull request Feb 20, 2019
7 tasks
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.

None yet

2 participants