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

Clarify that const is being cast away #302

Closed
wants to merge 2 commits into from

Conversation

tacaswell
Copy link
Contributor

@tacaswell tacaswell commented Apr 30, 2022

Closes #301

I should preface this with a disclaimer that my c++ is very rusty and this is the first time I have looked at this code base. I expect this is not the correct fix, but it compiles! I get one failure locally (see below).

python/cpython#91959 switch to using "new-style" c++ casts which are (apparently) stricter about casts.

One set of changes is to explicitly strip const when passing into the C API (to increase / decrease reference counts). I think this used to work because const-ness was being ignored before. Although this was fixed upstream to work, I think it is better to still have the explicit casting.

The other set of changes is to borrow the PyObject pointer from the greenlet wrapper classes. I think this used to work because the pointer was the first element in the wrapped objected so it is safe to directly cast it, however the new-style casting balks. (dropped this commit)

@tacaswell
Copy link
Contributor Author

I get one test failure locally:

===================================================================================================== FAILURES =====================================================================================================
___________________________________________________________________________ TestGreenlet.test_dealloc_catches_GreenletExit_throws_other ____________________________________________________________________________

self = <greenlet.tests.test_greenlet.TestGreenlet testMethod=test_dealloc_catches_GreenletExit_throws_other>

    def test_dealloc_catches_GreenletExit_throws_other(self):
        def run():
            try:
                greenlet.getcurrent().parent.switch()
            except greenlet.GreenletExit:
                raise SomeError
    
        g = greenlet(run)
        g.switch()
        # Destroying the only reference to the greenlet causes it
        # to get GreenletExit; when it in turn raises, even though we're the parent
        # we don't get the exception, it just gets printed.
        # When we run on 3.8 only, we can use sys.unraisablehook
        oldstderr = sys.stderr
        try:
            from cStringIO import StringIO
        except ImportError:
            from io import StringIO
        stderr = sys.stderr = StringIO()
        try:
            del g
        finally:
            sys.stderr = oldstderr
    
        v = stderr.getvalue()
>       self.assertIn("Exception", v)
E       AssertionError: 'Exception' not found in ''

src/greenlet/tests/test_greenlet.py:214: AssertionError
================================================================================================= warnings summary =================================================================================================
src/greenlet/tests/test_greenlet.py::TestGreenlet::test_dealloc_catches_GreenletExit_throws_other
  /home/tcaswell/.virtualenvs/bleeding/lib/python3.11/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: <greenlet.greenlet object at 0x7f6e668eddc0 (otid=0x7f6e668589f0) dead>
  
  Traceback (most recent call last):
    File "/home/tcaswell/source/p/python-greenlet/greenlet/src/greenlet/tests/test_greenlet.py", line 192, in run
      greenlet.getcurrent().parent.switch()
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  greenlet.GreenletExit: Killing the greenlet because all references have vanished.
  
  During handling of the above exception, another exception occurred:
  
  Traceback (most recent call last):
    File "/home/tcaswell/source/p/python-greenlet/greenlet/src/greenlet/tests/test_greenlet.py", line 194, in run
      raise SomeError
      ^^^^^^^^^^^^^^^
  greenlet.tests.test_greenlet.SomeError
  
    warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================================================================= short test summary info ==============================================================================================
FAILED src/greenlet/tests/test_greenlet.py::TestGreenlet::test_dealloc_catches_GreenletExit_throws_other - AssertionError: 'Exception' not found in ''

@tacaswell
Copy link
Contributor Author

9fe8746 is no longer needed due to CPython considering this a regression and fixing in python/cpython#92138

@tacaswell
Copy link
Contributor Author

This looks like it going to be fully fixed upstream, but I think you should keep the first two commits (to make in clear in the code that const is being discarded even though CPython will do the discarding for you).

@jamadden
Copy link
Contributor

Alright, thank you! I should actually have some time to look closely at this "soon"; I'm in the process of getting the time allotted by $WORK.

@tacaswell
Copy link
Contributor Author

tacaswell commented May 20, 2022

python/cpython#92951 is the upstream PR to watch.

@vstinner
Copy link
Contributor

python/cpython#92951 is the upstream PR to watch.

It got merged, so I suggest closing this PR which will no longer be needed with the future Python 3.11 beta2.

@tacaswell tacaswell changed the title Fix py311 cpp Clarify that const is being cast away May 23, 2022
@tacaswell
Copy link
Contributor Author

I dropped the commit that did the borrrowing, but I still think that it would be good to the explicit const casting in greenlets for clarity (rather than compatibility).

Leaving this to the greenlet developers to take or leave at their discretion.

@vstinner
Copy link
Contributor

I suggest you dropping the usage of const PyObject* in greenlet. See python/cpython#91768

jamadden added a commit that referenced this pull request Jan 27, 2023
See also python/cpython#91768

Fixes #302

Also stops building the mac wheels we ship with Ofast and debugging assertions. That's not meant for production use.
jamadden added a commit that referenced this pull request Jan 27, 2023
See also python/cpython#91768

Fixes #302

Also stops building the mac wheels we ship with Ofast and debugging assertions. That's not meant for production use.
@tacaswell tacaswell deleted the fix_py311_cpp branch January 27, 2023 22:55
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.

py311 issues with const correctness
3 participants