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 reference leak in Markup.join C implementation #47

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

hodgestar
Copy link
Contributor

When support for iterator arguments was added to the Markup.join implementation in 99ad51d the temporary tuple was replaced by a temporary list and a switch was made from PyTuple_SET_ITEM (which steals a reference) to PyList_Append (which does not) without adding the necessary Py_DECREF for the added item.

This PR adds the necessary reference.

Thanks you @eug3nix for reporting the issue in #46 and helping debug it. Would you mind checking that this PR solves your original issue?

@eug3nix
Copy link

eug3nix commented Sep 3, 2021

I have installed the updated version and can confirm that the issue is resolved. Thank you @hodgestar !

@hodgestar
Copy link
Contributor Author

Bleh. I forgot Travis went away so I should move the CI to GitHub Actions before merging this. I'll try do that over the weekend.

@rjollos
Copy link
Member

rjollos commented Sep 3, 2021

Bleh. I forgot Travis went away so I should move the CI to GitHub Actions before merging this. I'll try do that over the weekend.

It was migrated here:
https://app.travis-ci.com/github/edgewall/genshi

But appears the last 4 commits to master haven't triggered a build.

@hodgestar
Copy link
Contributor Author

But appears the last 4 commits to master haven't triggered a build.

Exactly. :/

@FelixSchwarz
Copy link
Member

In Travis I see something like "Could not authorize build request for edgewall/genshi."
Also other Travis did run for other github projects of mine a few weeks ago so I think this more of a genshi setup issue. Maybe try to reauthorize Travis?

@hodgestar
Copy link
Contributor Author

In Travis I see something like "Could not authorize build request for edgewall/genshi."

Thanks @FelixSchwarz. That led me to https://stackoverflow.com/questions/41034694/travis-could-not-authorize-build-request/41078031 and indeed no billing plan had been selected for the Edgewall organization. I selected the Free plan. Hopefully that was the right thing to do. :)

@hodgestar
Copy link
Contributor Author

All the builds have passed in https://app.travis-ci.com/github/edgewall/genshi/jobs/535510039 which I triggered manually, so I'm going to merge this. The PR itself doesn't seem to have been updated with the results, but maybe that will happen automatically on commits again in future.

@hodgestar hodgestar merged commit 5d84b83 into master Sep 3, 2021
@hodgestar hodgestar deleted the feature/fix-refleak-in-markup-join-gh46 branch September 17, 2021 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants