[3.8] bpo-39016: type_mro_modified produces negative refcount by ctismer · Pull Request #17555 · python/cpython · GitHub
Skip to content

[3.8] bpo-39016: type_mro_modified produces negative refcount - #17555

Closed
ctismer wants to merge 1 commit into
python:3.8from
pyside:3.8-refcount
Closed

[3.8] bpo-39016: type_mro_modified produces negative refcount#17555
ctismer wants to merge 1 commit into
python:3.8from
pyside:3.8-refcount

Conversation

@ctismer

@ctismer ctismer commented Dec 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

In the "if (custom) {" branch, lookup_maybe_method, which uses
borrowed references, is not handled correctly.
Solved by removing all PyXDECREF calls.

https://bugs.python.org/issue39016

@ctismer
type_mro_modified produces negative refcount
fa2c8fc 
It the "if (custom) {" branch, lookup_maybe_method, which uses
borrowed references, is not handled correctly.
Solved by removing all PyXDECREF calls.
@pablogsal
pablogsal changed the base branch from 3.8 to master December 10, 2019 16:20
@pablogsal
pablogsal changed the base branch from master to 3.8 December 10, 2019 16:20
@pablogsal

pablogsal commented Dec 10, 2019

Copy link
Copy Markdown
Member

@pablogsal pablogsal closed this Dec 10, 2019
@ambv ambv reopened this Dec 10, 2019
@pablogsal pablogsal changed the title bpo-39016: type_mro_modified produces negative refcount [3.8] bpo-39016: type_mro_modified produces negative refcount Dec 10, 2019
@ctismer

ctismer commented Dec 10, 2019

Copy link
Copy Markdown
Contributor Author

No guys, this "if (custom)" part is only in the 3.8 branch.
It makes no sense to use master. Maybe this is just a leftover and should be removed?

@pablogsal

pablogsal commented Dec 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

No guys, this "if (custom)" part is only in the 3.8 branch.
It makes no sense to use master.

Yeah, apologies for the confusion. I answered in the issue as well.

This still needs a NEWS entry and ideally a test.

@vstinner

vstinner commented Dec 10, 2019

Copy link
Copy Markdown
Member

No guys, this "if (custom)" part is only in the 3.8 branch.

Would it make any sense to reuse master code in 3.8?

@ctismer

ctismer commented Dec 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

No guys, this "if (custom)" part is only in the 3.8 branch.

Would it make any sense to reuse master code in 3.8?

No idea. I was not aware that 3.8 and master diverge that much. Should the "if (custom) {" clause perhaps be adjusted/removed like in master?

Update: master is probably a bit behind Python 3.8, see the comment in type_mro_modified. So I would probably use the corrected 3.8 version (no Py_XDECREF's) and apply it to master as well.

It was a change by Julien Palard from 2019-05-26, 180dc1b
Not sure why that did not go into master. Was added with good intention but bad refcounting ;-)

@ambv

ambv commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

@JulienPalard, can you shed some light why your change only went into 3.8 and not into master?

@encukou

encukou commented Dec 11, 2019

Copy link
Copy Markdown
Member

There's some misunderstanding there?
Julien's change is in master, and this patch applies cleanly.

@encukou

encukou commented Dec 11, 2019

Copy link
Copy Markdown
Member

Unfortunately, this isn't the right fix :(
Let's discuss more in the issue.

@ctismer

ctismer commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

@ctismer ctismer closed this Dec 11, 2019
@ctismer
ctismer deleted the 3.8-refcount branch December 11, 2019 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ctismer @pablogsal @vstinner @ambv @encukou @the-knights-who-say-ni @bedevere-bot