Menu

#328 FIX [ swig-Bugs-3541652 ] Refcount (SmartPointer / Implicit

open
nobody
general (37)
5
2022-01-28
2012-09-24
Solal Jacob
No

I submitted bugs 3541652 , and I try to fixed it. Also I'm not sure if it's the best way to fix it because I only need to comment/remove two lines of code which for me doesn't do anything (and I'm probably wrong) other than making Swig_Python_ConvertPtrAndOwn unthread-safe. If I'm wrong please give some advices to fix it properly.

Thanks.

(I included the patch and a test-case based on swig 2.0.8

Discussion

  • Solal Jacob

    Solal Jacob - 2012-09-24

    implicit conv patch

     
  • Solal Jacob

    Solal Jacob - 2012-09-24

    It seem that PyObject_CallFunctionObjAgs call Swig_Python_ConvertPtrAndOwn recursively so the patch could certainly broke thins in certain case. I think we must found a way to pass the implicitconv flags (to stop recursion), in a thread safe way.

     
  • Solal Jacob

    Solal Jacob - 2012-09-26

    The file 'second path attempt' avoid the problem of recursion. Instead of setting the implicitconv flags in SwigPyClientData structure it set the flags in the SwigPyObject structure. So the patch this patch avoid the race conditions and the problem of recursion. There is two new test case that show the patch work, but the first test case will still not work as it assign the same object and so it come as using a kind of global flags, but this test case is not really realistic and the variable will still be assigned.
    Also, I didn't provide the test case for the recursion problem but I tested it on a larger project who use refcounted Variant and the problems of recursion and race condition disappeared.

     
  • Solal Jacob

    Solal Jacob - 2012-09-26

    Second patch attempt

     
  • William Fulton

    William Fulton - 2012-12-08

    Looking at the patch I see a problem as it breaks the %implicitconv feature. This feature is 'documented' in the CHANGES file if you are not aware of it. I have just added a runtime test for the %implicitconv featurein revision 13950 on trunk as we were missing one. It demonstrates the breakage.

    Your patch can also be improved by changing:
    if (data && !sobj->implicitconv) {
    to
    if (sobj && !sobj->implicitconv) {

    which will at least fix the segfault when running the Python implicittest testcase.

    Could you rework your patch? Thanks.

     
  • Solal Jacob

    Solal Jacob - 2012-12-10

    Third patch attempt

     
  • Solal Jacob

    Solal Jacob - 2012-12-10

    Thanks for the reply,

    I joined a patch who use 'thread static local variable' to track the implicitconv state and to avoid race condition in multithread.
    This work fine for me and it seem to work with 'implicit_runme.py' the only drawback is that's compiler specific so it must fallback to a 'no thread storage' if the right compiler is not found.

     
  • Olly Betts

    Olly Betts - 2022-01-28

    The bug report URL is now https://sourceforge.net/p/swig/bugs/1256/

    It seems this never got resolved, I suspect because of the non-portable thread storage part.

    Python actually provides a thread local storage abstraction (actually two - seems there's a new "TSS" from Python 3.7 and a now-deprecated TLS in older versions) which would be a good way to address that.

     

Log in to post a comment.