Skip to content

bpo-35983: skip trashcan for subclasses #11841

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 10, 2019
Merged

bpo-35983: skip trashcan for subclasses #11841

merged 2 commits into from
May 10, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Feb 13, 2019 •
edited by bedevere-bot
Loading

@jdemeyer jdemeyer requested a review from 1st1 as a code owner February 13, 2019 15:52
@jdemeyer jdemeyer force-pushed the trashcan branch 3 times, most recently from d3a2746 to 7c4c52d Compare February 14, 2019 08:26
@jdemeyer jdemeyer mentioned this pull request Feb 14, 2019
Closed
@jdemeyer jdemeyer force-pushed the trashcan branch 3 times, most recently from e7769b0 to 9b0b137 Compare February 14, 2019 09:41
@1st1 1st1 requested a review from pitrou February 14, 2019 19:37
@1st1
Copy link
Member

1st1 commented Feb 14, 2019

LGTM, but let another core dev review this before merge. @pitrou could you please take a look?

@jdemeyer
Copy link
Contributor Author

Please consider backporting this to 2.7 and 3.7

@serhiy-storchaka serhiy-storchaka self-requested a review February 17, 2019 06:21
@jdemeyer
Copy link
Contributor Author

Any news?

@pitrou
Copy link
Member

pitrou commented Mar 27, 2019

I was asked for review. Unfortunately this is a hairy topic and I would have to take a dive to make a proper review. Depending how confident other core developers are, this could be merged anyway. @1st1, @serhiy-storchaka what do you think?

@scoder perhaps you'd like to take a look?

@pitrou
Copy link
Member

pitrou commented Mar 27, 2019

(PS: one issue with the trashcan mechanism is that it triggers rarely, so any bugs may occur only in obscure conditions that we don't have access to...)

@scoder
Copy link
Contributor

scoder commented Mar 27, 2019

I'm absolutely not an expert in this area, but several things speak in favour of Jeroen's change:

  1. He found a case that currently does not work and creates user visible problems.
  2. He found code in CPython that already tries to work around this for a specific case.
  3. He managed to implement a test for this.
  4. He came up with an implementation that seems to use backwards compatible macros.
  5. Looking over the change, neither @1st1 nor I (and apparently also not @pitrou) could find anything obviously wrong with it, even though no-one of us felt confident enough to determine that it's definitely correct.
  6. The current implementation obviously is less correct, given that it has at least one bug, which is not currently covered by tests.

My vote is for merging it and allowing users to find problems during the alpha/beta cycle. Several major projects have integration builds with the latest CPython master branch these days.

pitrou
pitrou requested changes Mar 27, 2019
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Here is a review after all.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka
serhiy-storchaka reviewed Mar 27, 2019
View reviewed changes
@jdemeyer
Copy link
Contributor Author

If you all agree, please remove the DO-NOT-MERGE label.

@pitrou
Copy link
Member

pitrou commented Mar 27, 2019

@serhiy-storchaka is the last one that needs approving.

@pitrou
Copy link
Member

pitrou commented Mar 28, 2019

Btw, if we are confident enough this could be a backport candidate.

@jdemeyer jdemeyer mentioned this pull request Mar 28, 2019
Closed
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Apr 1, 2019

By https://discuss.python.org/t/vote-to-promote-stefan-behnel-as-a-core-developer/1054 it looks like this will have positive reviews from no less than 4 core developers.

So @serhiy-storchaka could you please say more concretely why you do not approve?

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Apr 5, 2019

Btw, if we are confident enough this could be a backport candidate.

I'm seeing actual crashes caused by this on Python 2.7, so I'm interested in backporting to 2.7. The patch doesn't apply cleanly on top of 2.7, but I'm willing to work on that.

@pitrou
Copy link
Member

pitrou commented Apr 5, 2019

@serhiy-storchaka If you don't mind, I will merge this soon.

@jdemeyer jdemeyer mentioned this pull request Apr 5, 2019
Closed
@pitrou
Copy link
Member

pitrou commented Apr 5, 2019

I think @benjaminp will have to decide whether this is 2.7-backport-worthy or not.

serhiy-storchaka
serhiy-storchaka reviewed Apr 6, 2019
View reviewed changes
Include/object.h
... The body of the deallocator goes here, including all calls ...
... to Py_DECREF on contained objects. ...
Py_TRASHCAN_SAFE_END(p)
Py_TRASHCAN_END // there should be no code after this
Copy link
Member

Choose a reason for hiding this comment

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

Why Py_TRASHCAN_SAFE_END has been renamed to Py_TRASHCAN_END? Is it not safe anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why Py_TRASHCAN_SAFE_END has been renamed to Py_TRASHCAN_END? Is it not safe anymore?

I changed the signature, so I had to change the name too. Py_TRASHCAN_BEGIN and Py_TRASHCAN_END then seemed like the most logical names.

Copy link
Member

Choose a reason for hiding this comment

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

Could not you preserve the signature of Py_TRASHCAN_SAFE_END? This would minimize changes.

Having two sets of names with and without SAFE gives false hint that the SAFE pair is more preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not you preserve the signature of Py_TRASHCAN_SAFE_END?

Of course I could, but what's the point of passing an argument which is not used?

This would minimize changes.

Why is that a goal? My goal is producing a good and simple trashcan implementation, not to minimize changes.

Having two sets of names with and without SAFE gives false hint that the SAFE pair is more preferable.

When reading the comments in the code (the only place where these are documented), it's clear that one should use Py_TRASHCAN_BEGIN and that Py_TRASHCAN_SAFE_BEGIN is only meant for backwards compatibility.

This is all bikeshedding of course and I don't really care much about the name and the unused arguments. So, if you have a concrete proposal that you're willing to accept, just say it.

serhiy-storchaka
serhiy-storchaka reviewed Apr 6, 2019
View reviewed changes
jdemeyer added a commit to jdemeyer/cpython that referenced this pull request Apr 8, 2019
@jdemeyer
[2.7] bpo-35983: skip trashcan for subclasses (pythonGH-11841)
0c914c2
jdemeyer added a commit to jdemeyer/cpython that referenced this pull request Apr 8, 2019
@jdemeyer
[3.7] bpo-35983: skip trashcan for subclasses (pythonGH-11841)
c19ed1c
@jdemeyer
Copy link
Contributor Author

ping?

@pitrou
Copy link
Member

pitrou commented Apr 13, 2019

I'll let @serhiy-storchaka answer your last questions.

@pitrou
Copy link
Member

pitrou commented May 10, 2019

There's no need to hold this up any longer. I'm merging.

@pitrou pitrou merged commit 351c674 into python:master May 10, 2019
@pitrou
Copy link
Member

pitrou commented May 10, 2019

Thanks for being persistent @jdemeyer :-)

@jdemeyer jdemeyer deleted the trashcan branch May 10, 2019 17:25
@ambv ambv removed the DO-NOT-MERGE label Aug 16, 2021
@jdemeyer jdemeyer mentioned this pull request Jun 19, 2022
Closed
@jdemeyer jdemeyer mentioned this pull request Jul 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scoder scoder scoder left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@methane methane methane approved these changes

@1st1 1st1 1st1 approved these changes

@pitrou pitrou pitrou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jdemeyer @1st1 @pitrou @scoder @bedevere-bot @methane @serhiy-storchaka @ambv @the-knights-who-say-ni