map must be destroyed after kernels

[sogartar]

The library enforces order of destructors. Kernels' destructors must be called after map's destructor. I think this puts unnecessary burden on the user to keep the order correct. The problem is in MapBase::~MapBase() where kernels are checked if they are internally allocated. map should keep a separate list of it's internal kernels and not expect that kernels are still alive in its destructor.

[jonathan-beard]

I'm not sure I see the use case. Could you provide an example where the order must be maintained that's outside of what is expected of C++?

Here's the mapbase.cpp destructor code:

 40 MapBase::~MapBase()
 41 {
 42    auto &container( all_kernels.acquire() );
 43    for( raft::kernel *kern : container )
 44    {
 45       if( kern != nullptr && kern->internal_alloc )
 46       {
 47          if( kern->internal_alloc )
 48          {
 49             delete( kern );
 50          }
 51       }
 52    }
 53    all_kernels.release();
 54 }

At line 45 we're checking to see if the pointer is already NULL, which would indicate the runtime has already called the destructor at some point (and set it null after the fact). The internal allocate ensures that we're only deleting ones that we've created ourselves....the others should be deleted on the stack when the stack frame returns and the local destructors are called.

Could you post a code example that'd require some other ordering or break the current ordering constraints? I've played around with differing orders for kernel destruction but I can't quite come up with one that'd break the current framework. Thanks!!

[ymadzhunkov]

Consider the following:

{
   raft::map m;
   kernel a, b;
   m += a >> b;
}

construction order is m, a, b; destruction order is b, a, m

When the destructor of map is executed, the map holds pointers to objects/memory that is destroyed. Most the time, the value of this memory locations is not changed, but rarely the OS decided to pause the execution of that thread, between destruction of b, a and m. Then the OS caches the callstack, without caching a & b. When the execution the thread is restored, those locations are full will garbage data. virtual calls involve reading the virtual table of the class ( garbage in this case ).

For now we bypass it by declaring the map last, so it's destroyed before the kernels.

{
   kernel a, b;
   raft::map m;
   m += a >> b;
}

The solution is quite simple, keep a separate containers of internally allocated kernels.

[jonathan-beard]

got it...yes, that is an easy fix. will push a patch shortly. thanks for the explanation, I hadn't considered this case, good catch!

[jonathan-beard]

fixed.....there might be one more corner case related to this. That being said, can you build a test case where this bug can be triggered? I couldn't quite make it happen on either of my machines....there has to be an easier way...maybe deleting manually in the map to induce the behavior?