Function.vector() deleted prematurely when parent goes out of scope (python)

[Joachim Haga]

If I remove the v._dummy=x line in the function below, I get a segfault when trying to use the returned vector. I can create a self-contained test if necessary, but the problem looks obvious (f gets deleted while its vector data is still in use).

    def _as_vector(self, x):
        if isinstance(x, GenericVector):
            return x
[...]
        f = interpolate(x, self.space)
        v = f.vector()
        v._dummy = f
        return v

[Garth Wells (garth-wells)]

The problem is that Function returns a reference to the vector rather than a shared pointer. The solution are

1. Change Function such that it returns a shared_ptr. This is my preferred option. I don't believe that it will unduly complicate the C++ interface since in typical usage a user can do

  solve(A, *u.vector(), b);

in place of

  solve(A, u.vector(), b);

2. Add two more functions to Function called vector_ptr() and boiler plate to the SWIG interface to ignore and rename. I don't like this because following this option consistently we'll end up with four functions instead of two throughout the library and we'll always have to remember to hand tweak the SWIG interface to a avoid a memory bug.

[Joachim Haga (jobh)]

There's also the hack used in several places already, to do the _dummy
assignment in the dolfin python layer. Low effort, low risk, low elegance.
;) Python handles the circular dependencies fine.

I would also prefer shared_ptr to be used in general for any values that
may be held on to (i.e., all return values except possibly factory
functions, and any arguments that may be stored beyond the function call).

-j.

[Garth Wells (garth-wells)]

On 11 November 2011 11:12, Joachim Haga <email address hidden> wrote:
> There's also the hack used in several places already, to do the _dummy
> assignment in the dolfin python layer. Low effort, low risk, low elegance.
> ;) Python handles the circular dependencies fine.
>

It does though defeat the purpose of using shared_ptr in the first
place. I've also seen examples where the hacks keep something in scope
that should be destroyed.

> I would also prefer shared_ptr to be used in general for any values that
> may be held on to (i.e., all return values except possibly factory
> functions, and any arguments that may be stored beyond the function call).
>

I agree, and yes, we should update the factories to return shared
pointers rather than plain pointers.

I'll give Anders a chance to voice his opposition (we've had this
discussion before . . . .) before I commit a fix.

Garth

> -j.
>
> --
> You received this bug notification because you are a member of DOLFIN
> Core Team, which is subscribed to DOLFIN.
> https://bugs.launchpad.net/bugs/889021
>
> Title:
>  Function.vector() deleted prematurely when parent goes out of scope
>  (python)
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/dolfin/+bug/889021/+subscriptions
>

[Anders Logg (logg)]

On Fri, Nov 11, 2011 at 10:49:06AM -0000, Garth Wells wrote:
> The problem is that Function returns a reference to the vector rather
> than a shared pointer. The solution are
>
> 1. Change Function such that it returns a shared_ptr. This is my
> preferred option. I don't believe that it will unduly complicate the C++
> interface since in typical usage a user can do
>
> solve(A, *u.vector(), b);
>
> in place of
>
> solve(A, u.vector(), b);

We can add a shared_ptr interface to solve() so the * is not
necessary.

> 2. Add two more functions to Function called vector_ptr() and boiler
> plate to the SWIG interface to ignore and rename. I don't like this
> because following this option consistently we'll end up with four
> functions instead of two throughout the library and we'll always have to
> remember to hand tweak the SWIG interface to a avoid a memory bug.

I think it's important that one can program (simple) solvers in C++,
essentially all the demos, without needing to write "shared_ptr"
anywhere in the code. As long as we can do this, it doesn't matter
which option (1) or (2) we use.

--
Anders

> ** Changed in: dolfin
> Status: New => Confirmed
>
> ** Changed in: dolfin
> Milestone: None => 1.0-rc1
>
> ** Changed in: dolfin
> Importance: Undecided => High
>