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

open
nobody
general (37)
5
2012-12-21
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

     
    Attachments
  • 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.