Skip to content

[smart_holder] Add gil_scoped_acquire to shared_ptr_trampoline_self_life_support ctor. #4196

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 3 commits into from
Oct 7, 2022
Merged

[smart_holder] Add gil_scoped_acquire to shared_ptr_trampoline_self_life_support ctor. #4196

merged 3 commits into from
Oct 7, 2022
+9 −2

Conversation

@rwgk
Copy link
Collaborator

@rwgk rwgk commented Sep 25, 2022 •
edited
Loading

Description

This change fixes 9 failing targets discovered via Google-internal global testing with added PyGILState_Check() asserts in Py_INCREF() and Py_DECREF() (diff below).

Python 3.9 Include/object.h:

 PyAPI_FUNC(void) _Py_Dealloc(PyObject *);

+PyAPI_FUNC(int) PyGILState_Check(void); /* Include/cpython/pystate.h */
+
 static inline void _Py_INCREF(PyObject *op)
 {
+    assert(PyGILState_Check());
 #ifdef Py_REF_DEBUG
     _Py_RefTotal++;
 #endif
@@ -433,6 +436,7 @@ static inline void _Py_DECREF(
 #endif
     PyObject *op)
 {
+    assert(PyGILState_Check());
 #ifdef Py_REF_DEBUG
     _Py_RefTotal--;
 #endif

Internal testing ID: OCL:441666819:BASE:476672625:1664085876295:2e44fcc

Suggested changelog entry:



    

  
  



      


          

          
  
  

    
  
    
    

  

  

  



          

        
      




  




      

       
            



      

  
        





      

  
        





      

  
        

    

  
  

  
          rwgk
  

        added 2 commits
        October 7, 2022 11:36


  

      

  
  

  
          

  

    

      


  

      
        @rwgk
  




      

        
          Add test exercising fix & validation that the fix is needed (i.e. thi…
        

          
                        
      


      

        

    
    
    

  

      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          07118c8
        
      

    

  

    

      
…s is BROKEN).

test_class_sh_trampoline_shared_ptr_cpp_arg.py::test_std_make_shared_factory[pass_through_shd_ptr] PASSED                            [ 87%]
test_class_sh_trampoline_shared_ptr_cpp_arg.py::test_std_make_shared_factory[pass_through_shd_ptr_release_gil] FAILED                [100%]

```
================================================================= FAILURES =================================================================
______________________________________ test_std_make_shared_factory[pass_through_shd_ptr_release_gil] ______________________________________

pass_through_func = <built-in method pass_through_shd_ptr_release_gil of PyCapsule object at 0x7f1b209707b0>

    @pytest.mark.parametrize(
        "pass_through_func", [m.pass_through_shd_ptr, m.pass_through_shd_ptr_release_gil]
    )
    def test_std_make_shared_factory(pass_through_func):
        class PyChild(m.SpBase):
            def __init__(self):
                super().__init__(0)

        obj = PyChild()
        while True:
>           assert pass_through_func(obj) is obj
E           RuntimeError: NEEDED HERE: gil_scoped_acquire gil;
```

    







      

  
  

  
          

  

    

      


  

      
        @rwgk
  




      

        
          Put back fix.
        

      


      

        

    
    
    

  

      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          af28552
        
      

    

  







  








      

  
        





      

  
          

  
  

  
      @rwgk
  rwgk


        marked this pull request as ready for review

    October 7, 2022 19:24    
    




  
 
  

    
  

  



        @rwgk
  rwgk



        merged commit 59ef530
      into

      
  
    pybind:smart_holder
  


    Oct 7, 2022

  







  

  
  

  
      @rwgk
  rwgk


    
    deleted the
    
      
        shared_ptr_trampoline_self_life_support_ctor_gil_scoped_acquire
    
    branch

    October 7, 2022 19:24    
    




    

    

      
    

    


      


          @github-actions
github-actions
bot



          added
  the 

  needs changelog

  Possibly needs a changelog entry
 label


      Oct 7, 2022

    

  



    

    

      
    

    


      


          @rwgk
rwgk




          removed
  the 

  needs changelog

  Possibly needs a changelog entry
 label


      Oct 7, 2022

    

  



  

  

    
  


  

      


    @rwgk
rwgk



    mentioned this pull request
    
      Feb 11, 2023
    





  


  

      

  
  Closed

  







  













  
  

    

      

  




      


    


    

      

        
      

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  



      

    

  




  
          



      

  

    
    

    

  

    Reviewers
  


    


        

    


  
      @Skylion007
  
    Skylion007



    
      
        
      
    
    Skylion007 approved these changes

  



      

        
          +1 more reviewer
          
          
        
          

              

    


  
      @wangxf123456
  
    wangxf123456



    
      
        
      
    
    wangxf123456 approved these changes

  


          

      

      Reviewers whose approvals may not affect merge requirements
      






    

    

  


      
  

    Assignees
  



        


    No one assigned







      


  


  

    Labels
  



    

    None yet







      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      No milestone





    
      
          


  

    

      
        

          
  

    Development
  



            


Successfully merging this pull request may close these issues.





  
  



      
    

  




      

    

  
      

  

    

      3 participants
    

    

        
          @rwgk 
        
          @Skylion007 
        
          @wangxf123456