Ban pointers or references on classes?

Originally Posted by Daved

It still has nothing to do with pass by reference and everything to do with bad coding with threads.

It has all to do with references and none to do with threads.

Here is the code again without the functions, exhibiting the same behavior:

That's because it's a WRAPPER function. It is NOT supposed to do any work. It's there ONLY because calling CreateThread requires a specific function prototype and can only pass by pointer.
Stop modifying it. It's not supposed to do anything other than call the other functions.
Do I really have to write a true sample to illustrate this problem?

Even without the functions, you are sharing memory across threads, and one thread is deleting the memory while the other is using it.

There's no reason (except as a fun exercise) to try to ban use of references to your object.

The thing is that the thread want to use the same memory for some purpose (we're sharing data across threads).
Seeing as this is a safe pointer with a ref count, we're not deleting any data at all. Instead, what we're doing is decrementing the ref count. When the ref count reaches 0, it destroys the object.
The general rule is: For every instance of the object there is, the counter should increase with one. But it doesn't if you pass by reference of pointer.

What exactly is the sample doing?
It creates some memory that it wants the thread to get access to (because it needs it for some reason) and the main function should also use this memory to do some work with it.
The thread might be some sort of progress thread that reports progress to something by reading that data.
When the main function is done with whatever it's doing (does not need to report progress anymore, because it's finished), then it tells the class it doesn't need the memory anymore and the class recrements the ref pointer (and should naturally tell the thread it's done, as well).
The thread will in this case CRASH because the memory is freed (due to the object being passed by reference and not by value). If the object is passed by value, the ref counter will increase by one and will still be valid until the thread tosses it away too.

>> It has all to do with references and none to do with threads.
I can show the same crash without using references. Can you show the same crash without using threads?

>> That's because it's a WRAPPER function. It is NOT supposed to do any work.
That's not the point. You are saying that the flaw is in the passing of your object by reference to a function. I am saying that the flaw comes before that. The flaw comes when you share dynamic memory across threads and delete it in one of them. The fact that this "works" in foo3 is just luck because making a copy of the data early on avoids the crash. As iMalc pointed out, foo3 can cause a crash as well depending on the timing of events.

>> Seeing as this is a safe pointer with a ref count, we're not deleting any data at all. Instead, what
>> we're doing is decrementing the ref count. When the ref count reaches 0, it destroys the object.
Note the bolded parts above. They are contradictory. You are deleting the memory, just not directly. It still gets deleted.

>> For every instance of the object there is, the counter should increase with one. But it doesn't if you pass by reference of pointer.
You aren't understanding flow of control in a program. The counter doesn't increase if you don't call a function either. The "danger" of passing by reference is exactly the same as the danger of using the variable in the calling code. That's why I remove the functions. I'm proving the point that the same scenario can occur without the functions. The flaw is not in the calling of the functions, the flaw is earlier.


In your scenario, the simple problem is that you aren't incrementing the reference count when passing the pointer to the new thread. If you did that, the code would work no matter what. If you remove the thread, the code will work no matter what. The problem is the handling of the thread, not the functions.

I can argue against all that.

Originally Posted by Daved

>> That's because it's a WRAPPER function. It is NOT supposed to do any work.
That's not the point. You are saying that the flaw is in the passing of your object by reference to a function. I am saying that the flaw comes before that. The flaw comes when you share dynamic memory across threads and delete it in one of them. The fact that this "works" in foo3 is just luck because making a copy of the data early on avoids the crash. As iMalc pointed out, foo3 can cause a crash as well depending on the timing of events.

No, that is the whole point.
When passing a memory by a smart pointer, I can delete it an infinite number of times without freeing it when there's still a copy around somewhere.
And once again, I'm not deleting the memory.
The point is as with any safe program that the wrapper function takes the data and passes it on directly. I can even add thread safety to show you this. Do you want me to show you? I can modify the example above.

Originally Posted by Daved

>> Seeing as this is a safe pointer with a ref count, we're not deleting any data at all. Instead, what
>> we're doing is decrementing the ref count. When the ref count reaches 0, it destroys the object.
Note the bolded parts above. They are contradictory. You are deleting the memory, just not directly. It still gets deleted.

Correction: the functions do not delete the data. They're just telling the memory class: "I don't want this memory anymoer." The CLASS decides when to delete the data.

Originally Posted by Daved

>> For every instance of the object there is, the counter should increase with one. But it doesn't if you pass by reference of pointer.
You aren't understanding flow of control in a program. The counter doesn't increase if you don't call a function either. The "danger" of passing by reference is exactly the same as the danger of using the variable in the calling code. That's why I remove the functions. I'm proving the point that the same scenario can occur without the functions. The flaw is not in the calling of the functions, the flaw is earlier.

What you're basically saying is that it's bad to pass data between threads - often a necessity.

Originally Posted by Daved

In your scenario, the simple problem is that you aren't incrementing the reference count when passing the pointer to the new thread. If you did that, the code would work no matter what. If you remove the thread, the code will work no matter what. The problem is the handling of the thread, not the functions.

That's also the thing. The ref counter increases automatically when passing by value, but not when passing by pointner and reference.

Alright. Now take a look at my new function again.
You notice I have added thread synchornization to make sure the main thread won't continue until the second thread has entered foo2 or foo3. This time I make sure that the first CANNOT decrease the reference count before the foo2 and foo3 functions.
foo2 will still crash because it uses a reference.
foo3 will not crash because it passed by value and the ref counter is now 2.

>> What you're basically saying is that it's bad to pass data between threads - often a necessity.
No, I'm saying you're doing it wrong. If you fixed your thread code and passed data between threads correctly, the function wouldn't cause a crash.

>> The ref counter increases automatically when passing by value, but not when passing by pointner and reference.
That only matters when passing to a new thread. It does not matter when passing to a function.

When you pass it to a function, it is impossible for the calling code to decrement the reference count. Therefore, it is safe to pass it to a function without incrementing the reference count.

When you pass it to a thread, because the two threads are running at the same time, it is possible for the original thread to decrement the reference count. Therefore, it is not safe to pass it to a thread without incrementing the reference count.

>> Alright. Now take a look at my new function again.
This changes nothing. All you did was force what you were trying to accomplish with the Sleep. You're still missing the point entirely.

You have to provide an example that fails without threads. The problem is still in the way you pass the data to your thread.

You need to make a copy of the data that you use in the new thread before the original thread decrements the reference count. That is the rule regardless of what foo2 or foo3 does. Your current code follows the rule in foo3, but not in foo2. The problem is that you aren't following that rule. There are a bunch of other ways for you to not follow that rule and cause a crash that have nothing to do with passing by reference. There's no reason to forbid pass by reference for this specific case and leave all the other cases that cause a crash. To fix the problem, just follow the rule in the first place.

Take a step back. You're looking at it from the point where you already have foo3, and then you're changing it to a reference and watching it break. What you're missing is that the code was broken in the first place, and changing it to a reference only exposed that problem.

These messages seem to just be going around in circles...

If you don't want to pass your object by reference, change the function so it doesn't take a reference.

If it's a function you can't change, what about writing a wrapper function like this:

Code:

void func( smart_ptr<int>& ptr )
{
   ...
}

void func_wrapper( smart_ptr<int> ptr )
{
   func( ptr );
}

That's not the point cpjust. Elysia thinks that passing the reference counted smart pointer to a function by reference is dangerous. However, it's not. It's only dangerous to pass a reference counted smart pointer to a thread by reference and use it after the original thread might decrement the reference counter.

The way I would (and have previously) handled such a situation is to AddRef the smart pointer right before the _beginthreadex, or in your case CreateThread call. You then have to confirm that the creation succeeded, and if it failed, decrease the refcount again.
Then inside the thread you can simply Attach to the pointer passed in to it, and when that smart pointer goes out of scope the corresponding Release gets called. This means not having to wait on the new thread in any way as the AddRef is done on the new thread's behalf already.

Also since it is now AddRed'd and wont be destroyed by the main thread then the spawned thread is free to use (const) references to the smart pointer which are of course a more efficient option because they don't needlessly invoke additional unneeded calls to AddRef and Release which have no net effect.

Originally Posted by Daved

When you pass it to a thread, because the two threads are running at the same time, it is possible for the original thread to decrement the reference count. Therefore, it is not safe to pass it to a thread without incrementing the reference count.

No, it is safe to do so. What I do is put the main thread into sleep and wait first. The thread entry calls the real function which takes the parameter by value. NOW I've basically passed the argument by value to the thread. Now I can wake the main thread.

>> Alright. Now take a look at my new function again.
This changes nothing. All you did was force what you were trying to accomplish with the Sleep. You're still missing the point entirely.

What I did is force the program to behave as it should. In my new code, it's impossible for the foo function to do anything funny because of a race condition. Now I'm sure that when the main thread wakes, the second thread is in foo2 or foo3 and the argument has been passed successfully to foo3 (not NOT to foo2 since it takes a reference).

You need to make a copy of the data that you use in the new thread before the original thread decrements the reference count. That is the rule regardless of what foo2 or foo3 does.

And that is WHY I call foo2 or foo3 directly from foo.
When calling foo3, I make a copy in that thread because it's by value.
When calling foo2, I don't make a copy because it's a reference.
I also force the main thread to sleep until I can make a copy (or not) of the argument.

Your current code follows the rule in foo3, but not in foo2. The problem is that you aren't following that rule. There are a bunch of other ways for you to not follow that rule and cause a crash that have nothing to do with passing by reference. There's no reason to forbid pass by reference for this specific case and leave all the other cases that cause a crash. To fix the problem, just follow the rule in the first place.

You argue it isn't a reference problem, but it IS! It doesn't matter how I did it, because the only difference between foo3 and foo2 is the argument, one takes by value and one by reference. Therefore, the reference is at fault, even if I utilized them incorrectly.
I have shown, in this example, however bad my code may look to you, that it's possible to crash the program because a function takes a reference.
Therefore it isn't safe, and I want to ban it.

Now then, why don't you show me an example of how YOU think the code should look like?
And cpjust: Of course I can avoid using references, but the discussion is about banning such usage WITH the class.

Originally Posted by Elysia

And that is WHY I call foo2 or foo3 directly from foo.
When calling foo3, I make a copy in that thread because it's by value.
When calling foo2, I don't make a copy because it's a reference. I also force the main thread to sleep until I can make a copy (or not) of the argument.

Ok. This part is correct. You have followed the rule with foo3 by ensuring that the original pointer is not destroyed by the original thread until after the new thread is done using it.

You claim that because a switch to foo2 now breaks that rule, that foo2 is dangerous. I'm saying that foo2 is only dangerous because you broke the rule by calling it.

You can break the rule without using foo2. You can use foo2 without breaking the rule. Therefore, the problem is not foo2.

Originally Posted by Elysia

You argue it isn't a reference problem, but it IS! It doesn't matter how I did it, because the only difference between foo3 and foo2 is the argument, one takes by value and one by reference. Therefore, the reference is at fault, even if I utilized them incorrectly.
I have shown, in this example, however bad my code may look to you, that it's possible to crash the program because a function takes a reference.
Therefore it isn't safe, and I want to ban it.

Your example shows that "with your reference counted smart pointer class passing by reference is dangerous and passing by value is not when you are in a threaded environment and when the value is guaranteed not to be corrupted by another thread until immediately after the function call and when the value passed is not used again after the function call."

I'll give you that. That's a lot different than "with your reference counted smart pointer class passing by reference is dangerous".


Look at this example. Using an int variable in the denominator of a division operation is dangerous:

Code:

void bar2()
{
  double numerator = 2.0;
  int denominator = 2.0 / 4.0;
  double quotient = numerator / denominator;
}
void bar3()
{
  double numerator = 2.0;
  double denominator = 2.0 / 4.0;
  double quotient = numerator / denominator;
}

int main()
{
  //bar2();
  bar3();
}

It doesn't matter how I did it, because the only difference between bar3 and bar2 is the type of the denominator, one uses double, the other uses int. Therefore, the int is at fault, even if I utilized them incorrectly.
I have shown, in this example, however bad my code may look to you, that it's possible to crash the program because a division operation uses an int in its denominator.
Therefore it isn't safe, and I want to ban it.

Hopefully you see the parallels to your example. The rule is don't divide by zero. You can break the rule without making the denominator an int. You can make the denominator an int and not break the rule. Therefore, the problem is not the fact that the denominator is an int.

>> Now then, why don't you show me an example of how YOU think the code should look like?
I would use iMalc's suggestion. Force a copy to be made so that the thread cannot possibly use the original pointer unless the reference count has been incremented.

Originally Posted by Daved

Ok. This part is correct. You have followed the rule with foo3 by ensuring that the original pointer is not destroyed by the original thread until after the new thread is done using it.

You claim that because a switch to foo2 now breaks that rule, that foo2 is dangerous. I'm saying that foo2 is only dangerous because you broke the rule by calling it.

You can break the rule without using foo2. You can use foo2 without breaking the rule. Therefore, the problem is not foo2.

Now here's the thing - perhaps I am doing things the wrong way, but regardless, this happens because I use references in a bad way. And I want to avoid such bugs by safe-guarding it.
I think you'll agree that it's a lot safer when every variable of the memory class is counted towards the ref count. Therefore, I feel it safer ONLY to pass by value then to allow the (potentially) dangerous pass by reference, if used incorrect.

Your example shows that "with your reference counted smart pointer class passing by reference is dangerous and passing by value is not when you are in a threaded environment and when the value is guaranteed not to be corrupted by another thread until immediately after the function call and when the value passed is not used again after the function call."

I'll give you that. That's a lot different than "with your reference counted smart pointer class passing by reference is dangerous".

But that's how data is usually passed between threads. I block the main thread until I'm gauranteed that the functions in the new thread have secured its access to the data. And with shared pointers, this is after the thread has made a local copy of the variable.

Look at this example. Using an int variable in the denominator of a division operation is dangerous:

It doesn't matter how I did it, because the only difference between bar3 and bar2 is the type of the denominator, one uses double, the other uses int. Therefore, the int is at fault, even if I utilized them incorrectly.
I have shown, in this example, however bad my code may look to you, that it's possible to crash the program because a division operation uses an int in its denominator.
Therefore it isn't safe, and I want to ban it.

Hopefully you see the parallels to your example. The rule is don't divide by zero. You can break the rule without making the denominator an int. You can make the denominator an int and not break the rule. Therefore, the problem is not the fact that the denominator is an int.

The thing here is that if I can avoid such bugs by banning such practice in some ways that do not ban needed functionality, I don't see why I shouldn't. Like adding "const" to varaibles that aren't supposed to be modified, by safe-guarding or banning some practices, I can find more bugs at design time.

>> Now then, why don't you show me an example of how YOU think the code should look like?
I would use iMalc's suggestion. Force a copy to be made so that the thread cannot possibly use the original pointer unless the reference count has been incremented.

That's only done through pass by value.

You say you block the main thread until you're guaranteed that the functions in the new thread have secured its access to the data. So why not do this:

Code:

HANDLE hEvent;

void foo2(pp<int>& pTest)
{
	Sleep(1000); // Simulate doing some work for 1s
	int n = *pTest;
	n;
}

DWORD WINAPI foo(void* pParameter) // Wrapper function; takes param and passes it to our processing functions
{
	// Thread entry. Fetch variable we passed along and pass it on to our processing functions.
	ppnew<int>* p = (ppnew<int>*)pParameter;

	pp<int> pThreadCopy = *p; // make a copy here
	SetEvent(hEvent); // Tell main thread to continue

	foo2(*pThreadCopy); // this works!
	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	//foo_s* ps = new foo_s;
	hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
	ppnew<int> OurMemory; // The memory we're fighting over
	*OurMemory = 100;
	//NewThread(&CKantanAnimeApp::foo, this, ps->pint);
	::CreateThread(NULL, NULL, &foo, &OurMemory, NULL, NULL); // Begin race condition
	WaitForSingleObject(hEvent, INFINITE); // Wait for the new thread to be ready
/*	Sleep(1000); // Simulate some work for 1s*/
	int n = *OurMemory;
	//delete ps;
	OurMemory = ppnew<int>; // We require some new memory now
				// and have no use for the old memory,
				// so allocate some new and discard the old.
	Sleep(100000); // Keep sleeping so the rest of the code won't
		       // execute, because it's not part of this example.
	n;
	...
}

That fixes the problem closer to the source.

You want to ban a practice that isn't essential because it can more easily expose a separate flaw in the program. That might be ok if the thing you want to ban is useless and/or easily encountered. I don't think either is the case here.

It's not easily encountered, because the only way the issue will come up is if you have multiple threads and one thread fails to make a copy of the object before passing it to the function or fails to ensure that the other thread will wait until it's safe to destroy the object. This might be your scenario now, but it's not common because in most cases you'll do the threading code correctly.

Passing by reference is not useless. If you wanted to update the actual value, then you would have to pass by reference. In your case, you are not modifying the value, so foo2 should really use a reference to const. However, a reference to const is also not useless. It is more efficient. Presumably since you are in a multithreaded environment you have synchronization around your reference counting in the smart pointer. (You don't want to check the reference count in one statement and delete the pointer in another or the reference count might change from another thread in between those statements and cause you to delete a referenced pointer.) Passing by reference to const avoids the reference counting and related synchronization that pass by value would require, so passing by reference to const is more efficient. That's why it isn't useless.

I understand your point of view. There are cases where things are banned even if they aren't inherently dangerous because they are useless and can expose other programming flaws (not allowing non-const references to temporaries is an example). I just think this is not one of those cases. You've got such a specific example that would be solved by proper coding practices that it isn't worth it to ban a marginally useful feature.


>> That's only done through pass by value.
I don't understand this. You should be passing by value to the thread. If that's not technically possible, then simulate it.

Originally Posted by Daved

You say you block the main thread until you're guaranteed that the functions in the new thread have secured its access to the data. So why not do this:

Code:

HANDLE hEvent;

void foo2(pp<int>& pTest)
{
	Sleep(1000); // Simulate doing some work for 1s
	int n = *pTest;
	n;
}

DWORD WINAPI foo(void* pParameter) // Wrapper function; takes param and passes it to our processing functions
{
	// Thread entry. Fetch variable we passed along and pass it on to our processing functions.
	ppnew<int>* p = (ppnew<int>*)pParameter;

	pp<int> pThreadCopy = *p; // make a copy here
	SetEvent(hEvent); // Tell main thread to continue

	foo2(*pThreadCopy); // this works!
	return 0;
}

BOOL CKantanAnimeApp::InitInstance()
{
	hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
	ppnew<int> OurMemory; // The memory we're fighting over
	*OurMemory = 100;
	::CreateThread(NULL, NULL, &foo, &OurMemory, NULL, NULL); // Begin race condition
	WaitForSingleObject(hEvent, INFINITE); // Wait for the new thread to be ready
	int n = *OurMemory;
	OurMemory = ppnew<int>; // We require some new memory now
				// and have no use for the old memory,
				// so allocate some new and discard the old.
	Sleep(100000); // Keep sleeping so the rest of the code won't
		       // execute, because it's not part of this example.
	n;
	...
}

That fixes the problem closer to the source.

Yes. Yes, it does.
But it's making a local copy and then passing it by reference.
In contrast, foo3 does exactly the same thing, it takes a local copy. Only this time it isn't a reference. I find it not an argument for references. It just includes additional security on your part, giving you more responsibility. Which is not the way a class should work - it should remove responsbility for you (handling it for you) and exposing its functionality to you.

Passing by reference is not useless. If you wanted to update the actual value, then you would have to pass by reference. In your case, you are not modifying the value, so foo2 should really use a reference to const. However, a reference to const is also not useless. It is more efficient.

But it also goes against the design of the class: for each variable of the class that points to the same memory, they will count towards ref count.
If you need to pass a memory handler object through reference, then you're not supposed to be using the memory handler at all.

Presumably since you are in a multithreaded environment you have synchronization around your reference counting in the smart pointer. (You don't want to check the reference count in one statement and delete the pointer in another or the reference count might change from another thread in between those statements and cause you to delete a referenced pointer.)
Passing by reference to const avoids the reference counting and related synchronization that pass by value would require, so passing by reference to const is more efficient. That's why it isn't useless.

Well, no, not yet. I haven't had the need to do so. And when I do, I will make it optional. If you believe you're going to use it across threads, then you activate it, and if not, then you don't. That save time of expensive locking and unlocking if it's not going to be used by another thread.

I understand your point of view. There are cases where things are banned even if they aren't inherently dangerous because they are useless and can expose other programming flaws (not allowing non-const references to temporaries is an example). I just think this is not one of those cases. You've got such a specific example that would be solved by proper coding practices that it isn't worth it to ban a marginally useful feature.

It's just a sample. And, in fact, it's a race condition example too. And race conditions are not very rare at all.
Though it seems that yes, as long as you make at least one copy of the variable for each thread, it will work as it should.
But again, I designed the class to be passed by value (overloaded a copy constructor & all).

Originally Posted by Elysia

Yes. Yes, it does.
But it's making a local copy and then passing it by reference.
In contrast, foo3 does exactly the same thing, it takes a local copy. Only this time it isn't a reference. I find it not an argument for references. It just includes additional security on your part, giving you more responsibility. Which is not the way a class should work - it should remove responsbility for you (handling it for you) and exposing its functionality to you.


But it also goes against the design of the class: for each variable of the class that points to the same memory, they will count towards ref count.
If you need to pass a memory handler object through reference, then you're not supposed to be using the memory handler at all.


Well, no, not yet. I haven't had the need to do so. And when I do, I will make it optional. If you believe you're going to use it across threads, then you activate it, and if not, then you don't. That save time of expensive locking and unlocking if it's not going to be used by another thread.


It's just a sample. And, in fact, it's a race condition example too. And race conditions are not very rare at all.
Though it seems that yes, as long as you make at least one copy of the variable for each thread, it will work as it should.
But again, I designed the class to be passed by value (overloaded a copy constructor & all).

By your design you have to make a copy of the smart pointer via pass-by-value at some point. Failing to do that at all really has no bearing on whether references are involved or not. You can just as easily not even call foo2 or foo3, and cause a problem in foo. If you rely on a copy being made, or more accurately you rely on the refcount being incremented, then simply do that at the time you create the thread. Don't make it possible for someone to screw it up by not invoking the copy-constructor of your smart pointer. This is the correct thing to do anyway since, if the new thread is to share ownership of the data then it is obligated to have the refcount incremented (and eventually decremented) once for it. This should not be delayed until the first function call within the new thread.
Another way of putting it, foo3 can't simply take ownership of something foo doesn't itself own.
Since you don't have any problem with unnecessary addref's and release's, then you most certainly can't have a problem with just one of each of these in the right place.

Oh I see Dave has already been trying to set you straight on this.
It's not more responsibility at all, it's less! It's preventing other code from having to try and take ownership of something that should already be owned by foo. It's not pushing the work that foo should be doing onto bits of code called by foo.
If you insist on doing it wrong though, so be it.

Originally Posted by iMalc

Oh I see Dave has already been trying to set you straight on this.
It's not more responsibility at all, it's less! It's preventing other code from having to try and take ownership of something that should already be owned by foo. It's not pushing the work that foo should be doing onto bits of code called by foo.
If you insist on doing it wrong though, so be it.

I can't see how that works. The ownership of the memory belongs to NONE but the class itself. Even if a object goes out of scope, the memory won't be freed until the ref count is 0.
And I also do not believe that having "special exception" functions is a good idea. From the sound of it, I need a function like PassByThread() to increase ref count or create another copy of the variable.

But regardless... if just fun fun's sake, is it possible to ban references and ALL creation of new instances of it on the heap?

Originally Posted by Elysia

But regardless... if just fun fun's sake, is it possible to ban references and ALL creation of new instances of it on the heap?

I can imagine no way to prevent creation of references in C++. To prevent creation of instances, use a private constructor.

If the language makes it so difficult to achieve X, perhaps achieving X is not a good idea.