SharedPtr - potential pitfall that can be easily fixed

Everything about the development of Urho3D.

SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 08 Apr 2016, 14:45

I ran into a problem that caused some annoying segfaults.

The Problem

Consider the following factory pattern (reduced for simplicity):

Code: Select all
class Item
{
public:
    static SharedPtr<Item> Item::create(Item_e item)
    {
        switch(item) {
            case ITEM_FLOWER: return new FlowerItem;
            /* etc */
        }
        return SharedPtr<Item>();
    }
};


Now consider the following call to this factory method:
Code: Select all
Item* item = Item::create(ITEM_FLOWER);
// Oh oh, the FlowerItem object was destroyed here


It shouldn't be possible to convert SharedPtr to a raw pointer like this. This basically makes SharedPtr::Get() a useless method.

Furthermore, although not as bad as above, these implicit conversions allow for somewhat confusing code. For example:
Code: Select all
SharedPtr<Foo> foo;
doAThing(foo);


Does doAThing(); take a raw pointer Foo* or a SharedPtr<Foo>? We cannot know without looking up the function signature.

Or this:
Code: Select all
Foo* MyClass::SomeMethod() { return foo_; }


Is foo_ a SharedPtr or a raw pointer? Again, we cannot know without looking at the definition of foo_.


Proposed solution

I propose to make the following change to SharedPtr and WeakPtr. Change this:
Code: Select all
operator T*() const { return ptr_; }

to this:
Code: Select all
operator const T*() const { return ptr_;}


This change will fix all of the problems explained above and at the same time it will still allow for code such as this:
Code: Select all
SharedPtr<Item> item = Item::create(ITEM_FLOWER);
if(!item) // This is still valid!
    return;


This change will force us to change these examples:
Code: Select all
// 1
SharedPtr<Foo> foo;
doAThing(foo);

// 2
Foo* MyClass::SomeMethod() { return foo_; }


Into this:
Code: Select all
// 1
SharedPtr<Foo> foo;
doAThing(foo.Get());

// 2
Foo* MyClass::SomeMethod() { return foo_.Get(); }


Which allows us to instantly see that if SharedPtr is being used or not.

If the dev team agrees, I shall go ahead and make this change.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by cadaver » 08 Apr 2016, 22:16

I would agree this is good, however I will reserve final judgement until I see the PR (and exact amount of engine changes.) WeakPtr should be safe to keep as is, as getting a raw ptr from a weak ptr should have no unintended consequences.
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 09 Apr 2016, 10:39

Good idea.

This change will fix all of the problems explained above and at the same time it will still allow for code such as this:
Code: Select all
    SharedPtr<Item> item = Item::create(ITEM_FLOWER);
    if(!item) // This is still valid!
        return;


One could also use "operator bool() const" which is usually done for such checks. The obvious archetype std::shared_ptr does that too: http://www.cplusplus.com/reference/memory/shared_ptr/

T* and SharedPtr<T> feel weirdly mixed in Urho. What is the guideline behind that? Is there a consistent style?
Is there a case of getting a T* from a function and being responsible for that object (having to delete it)? Would be weird. Should be a unique or shared pointer.
Is there a case of passing a T* to a function, returning from it and not being able to safely delete the T? Should be a shared or weak pointer then.
There could be more pitfalls which can be avoided with types and not having error prone implicit conversions like SharedPtr::operator T*().

Also functions like Terrain::SetHeightmap(Image*) could act differently depending on whether they got a naked pointer or a shared pointer. In the first case the ressource would be copied and managed internally (if it is actually needed after returning from the function of course). In the second case the same ressource could be shared across objects to save memory. Maybe that is already done internally, having that more explicit would be better though.
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 09 Apr 2016, 12:36

@cadaver - I will begin work on creating the PR in that case.

As a side note, I'm interested in where you first learned about intrusive reference counts? I've never seen anything quite like it before Urho3D.

gawag wrote:One could also use "operator bool() const" which is usually done for such checks. The obvious archetype std::shared_ptr does that too: http://www.cplusplus.com/reference/memory/shared_ptr/


This is a good idea. That means we can get rid of operator T*() const completely.

gawag wrote:T* and SharedPtr<T> feel weirdly mixed in Urho. What is the guideline behind that? Is there a consistent style?


It's dangerous to think of Urho's SharedPtr<T> as being analogous to std::shared_ptr<T> because they don't have the same semantic meaning. As I've come to understand, in Urho3D, everything inheriting from RefCounted is considered a shared resource, regardless of whether a function takes T* or SharedPtr<T> as an argument. For instance, I could easily write a function that manually increments the refcount:

Code: Select all
void foo(Bar* bar) {
    bar->AddRef();
}


You simply cannot make the assertion that just because a function takes a raw T* pointer it does not increment the refcount later on. You have to assume that every object is a shared object by default (since virtually everything in Urho3D inherits from RefCounted).

So with that in mind, Urho3D's guideline is to pass around raw pointers whenever possible (for speed), and if it so happens that an object wishes to store the pointer for later use, it will wrap it in a SharedPtr<T> in order to increment the refcount and stop it from potentially being deleted elsewhere.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by cadaver » 09 Apr 2016, 13:24

I no longer remember where I first saw intrusive pointers, possibly in kNet library.

The primary reasons Urho uses intrusive shared pointers is:
- Prevent situations where shared -> raw -> shared conversions could result in two separate refcounts being created, which later leads to double-delete and crash. Std::shared_ptr has make_shared mechanism to prevent this, but the classes must remember to inherit shared_from_this, and you must remember to use make_shared.
- Easiest interoperability with AngelScript handles, as the API can just hand out raw ptrs in all cases where it still owns the objects somewhere (like in the scene graph.)

In C++ application code IMO you should never do manual AddRef() or ReleaseRef(), but use shared ptrs. Script bindings would sometimes need to use these when "transferring" ownership from a shared ptr.
User avatar
cadaver
Urho3D author
Urho3D author
 
Posts: 1802
Joined: 16 Jan 2014, 14:52
Location: Finland

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by sabotage3d » 09 Apr 2016, 14:11

Just as a side note. Are going for the for something similar to the behaviour of Boost intrusive_ptr?
Boost intrusive_ptr: http://www.boost.org/doc/libs/1_57_0/libs/smart_ptr/intrusive_ptr.html
User avatar
sabotage3d
Have many posts
Have many posts
 
Posts: 515
Joined: 25 Oct 2014, 13:26

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 09 Apr 2016, 19:57

Ha! :lol: That's funny. This situation is kinda hilarious:

This is a good idea. That means we can get rid of operator T*() const completely.

I actually wrote a whole and long paragraph about it, thought about it for quite a while and removed it again. The "problem" I saw is that functions who take a specific object (like an Image or a Texture) may not really care how it is managed. From a user perspective you want to give them the resource independent if it's a T* or a SharedPtr<T> or a UniquePtr<T> or whatever. This can be either done by having a huge amount of overloads for every common T* or T* wrapper but one doesn't really want to do and maintain that.
The other option is the current existing one with the implicit conversion to T* in some way so that every function taking a T* does also directly accept SharedPtr<T> without having to use the .Get() (code bloat). I actually thought you thought about the same thing already and intentionally suggesting making it an implicit conversion to T* const as this could still allow giving functions taking an T* (or T* const) a SharedPtr<T>.

Actually just another option came to my mind but I would need to test that and it would be kinda odd... I'll test that...

Was the reason for the implicit SharedPtr<T> to T* conversion the passing to functions taking a T*? Am I missing something else?
The conversion is quite a big and dangerous pitfall but I see this one big reason for it and no perfect solution.

[all that SharedPtr and intrusive pointer stuff]

Eww. :? Really? That's totally unexpected. I thought it was similar to std::shared_ptr like the name suggested. I kinda assumed something like that happening in Urho but I didn't know exactly until now.
I see the reason for that but at least the naming is terrible. What about IntrusivePtr<T>? Is SharedPtr also used like a normal shared pointer (like std::shared_ptr) and not like an intrinsic pointer? Smells like two different use cases that suggest two different types.
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 10 Apr 2016, 00:20

I've researched the idea I mentioned: https://github.com/damu/wiki/blob/maste ... classes.md
If one wants to avoid having many overloads like:
Code: Select all
Terrain::SetHeightmap(Image*)
Terrain::SetHeightmap(SharedPtr<Image>)
Terrain::SetHeightmap(WeakPtr<Image>)
...

One could use the described technique of using a converter class:
Code: Select all
Terrain::SetHeightmap(AnyPtr<Image>)

With that the SetHeightmap function (and any other function taking a "AnyPtr") accepts all pointer types it is configured for like T*, SharedPtr<T> and WeakPtr<T>. Without having to have the wrapper implicitly be convertible to T*, that implicit conversion is type safe offloaded to AnyPtr.
I've included a screenshot of the assembler output. It's practically cost free, depending on the case it can even be cheaper.

Comments?
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by TheComet » 10 Apr 2016, 12:44

cadaver wrote:In C++ application code IMO you should never do manual AddRef() or ReleaseRef(), but use shared ptrs. Script bindings would sometimes need to use these when "transferring" ownership from a shared ptr.


Obviously, yeah. I just wanted to be as clear as possible in that example.

gawag wrote:I've researched the idea I mentioned: https://github.com/damu/wiki/blob/maste ... classes.md


There are many problems with your proposed solution using any_pointer. The TL;DR version is you're breaking semantic meaning and you're re-introducing the issue I explained in the first post:

Code: Select all
class Item
{
public:
    static SharedPtr<Item> Item::create(Item_e item)
    {
        switch(item) {
            case ITEM_FLOWER: return new FlowerItem;
            /* etc */
        }
        return SharedPtr<Item>();
    }
};

class Player
{
    Item* item_;
public:
    void HoldItem(AnyPtr<Item> item)
    {
        item_ = item;
    }
}


Here's the problem again:
Code: Select all
Player player;
player.HoldItem(Item::create(ITEM_FLOWER));
// Oh oh, player.item_ is now pointing to a destroyed object
// This is exactly what I'm trying to fix, AnyPtr re-introduces the issue


gawag wrote:One could use the described technique of using a converter class:
Code: Select all
Terrain::SetHeightmap(AnyPtr<Image>)

With that the SetHeightmap function (and any other function taking a "AnyPtr") accepts all pointer types it is configured for like T*, SharedPtr<T> and WeakPtr<T>. Without having to have the wrapper implicitly be convertible to T*, that implicit conversion is type safe offloaded to AnyPtr.
I've included a screenshot of the assembler output. It's practically cost free, depending on the case it can even be cheaper.


Again, it re-introduces the issue from my original post. As far as semantics go, what does Terrain::SetHeitmap(AnyPtr<Image>) mean semantically? Does Terrain own that resource afterwards? Or does Terrain hold a weak reference to it? Or does Terrain rely on the parent object's lifetime and simply hold a raw pointer?

The fact that Image is a RefCounted object makes it a shared object, end of story. It doesn't matter how many wrapper classes you create, you won't be able to change the semantic meaning. Like I tried to explained in my previous post, you are confusing SharedPtr<T> with std::shared_ptr.

What's wrong with just calling
Code: Select all
terrain->SetHeightMap(myImage.Get())

where SetHeigtMap() accepts a raw pointer Image*? It's clear that we're passing a raw pointer, and it's clear that since Image inherits RefCounted, it is a shared resource, meaning that Terrain could potentially gain ownership of the image passed. There is no confusion in this call.
I'm a non-binary non-cis sexually fluid cephalopod identifying genderqueer mocha frappé latte
User avatar
TheComet
Active user
Active user
 
Posts: 122
Joined: 29 Jan 2014, 14:07
Location: Germany

Re: SharedPtr - potential pitfall that can be easily fixed

PostPosted by gawag » 10 Apr 2016, 13:54

Ah, you got got me wrong there.
The idea of my AnyPtr is just to avoid overloads if one wants to not have a semantic meaning. Just as in "do stuff with X". For example blurring a given Image.
If one should pass a std::shared_ptr, the function should take a std::shared_ptr.
If the function should take a weak pointer, it should take a weak pointer.
The "dirty hack" that Urho is doing with it's T* when T is a child of RefCounted is pretty misleading as a naked pointer has no semantic meaning but the RefCounted naked pointer actually has a semantic meaning. Ideally there should be a type for this slightly different smart pointer.
The AnyPtr should be only used as a replacement for T* (with no semantic meaning) so that also a SharedPtr<T> (and maybe others as well) can be passed without requiring .Get().

To the player with the item example:
You are using an Item* but want to manage that resource. There's nothing hinting towards any kind of resource management as it is just a naked pointer. It would be clear if it would be a SharedPtr<Item> and the Item would be passed as a SharedPtr<Item>. We are in the age of smart pointers and having a naked pointer but wanting any kind of smart behavior from it can be considered a bug.

Again, it re-introduces the issue from my original post. As far as semantics go, what does Terrain::SetHeitmap(AnyPtr<Image>) mean semantically? Does Terrain own that resource afterwards? Or does Terrain hold a weak reference to it? Or does Terrain rely on the parent object's lifetime and simply hold a raw pointer?

In the ideal case: It doesn't do anything with that Image afterwards. It may have modified it or copied it but it is safe to delete that Image directly after SetHeightmap.
I just looked at the code and the SetHeightMapInternal sets a SharedPtr with that given image. So it is using the Image shared but that was not hinted in any way as it only got an Image* originally.This hidden smart pointer stuff is really dirty.

So it kinda boils down to:
- wanting shared ownership without costs at every argument pass
- ideally: wanting to indicate that the thing the pointer is passed to will use it shared (like the SetHeightmap) or not care at all after leaving the function

What's wrong with just calling
Code: Select all
    terrain->SetHeightMap(myImage.Get())

where SetHeigtMap() accepts a raw pointer Image*? It's clear that we're passing a raw pointer, and it's clear that since Image inherits RefCounted, it is a shared resource, meaning that Terrain could potentially gain ownership of the image passed. There is no confusion in this call.

Two problems: Currently .Get() is not required and removing the implicit cast in SharedPtr to T* would break code. Secondly requiring .Get() everywhere adds a lot of code bloat. Both could be avoided by for example my approach with a converter class.
Also having to distinguish between naked pointers in general and naked pointers with the base RefCounted is pretty misleading. As already said having a zero cost type (like no ref count changes when copied and similar) would be better as it would be also fast (the speed reason you mentioned) but less misleading and add type safety.

I'm currently not seeing a perfect way to solve the issues with functions like SetHeightmap that ideally should take a shared pointer but without the cost.
Seems to be a common issue:
http://stackoverflow.com/questions/8385 ... -reference
http://stackoverflow.com/questions/3275 ... shared-ptr
http://stackoverflow.com/questions/1082 ... -arguments
https://herbsutter.com/2013/06/05/gotw- ... arameters/

Though in the case of SetHeightmap the cost for passing a shared pointer by value can be neglected as that function isn't called that often. But there may be similar cases that are actually performance critical.
Besides the neglectable cost there's nothing speaking against Terrain::SetHeighmap(SharedPtr<Image>) am I right?

Hm one option would be Terrain::SetHeighmap(SharedPtr<Image>&). Zero cost at first but also "shareable" due to not being const. Still kinda dirty.
Old Unofficial Urho3D Wiki: http://urho3d.wikia.com/
"Newer" Unofficial Urho3D Wiki: https://urho3d.miraheze.org/
My GitHub: https://github.com/damu (changed my name recently from gawag to damu there)
User avatar
gawag
Active user
Active user
 
Posts: 192
Joined: 12 Feb 2015, 03:48
Location: Germany

Next

Return to Developer Talk

Who is online

Users browsing this forum: No registered users and 1 guest

cron