Problems with PooledEnumerable

[Smjert]

Hope i opened the thread in the right place.

I think there's a problem with PooledEnumerable Free function (not only with it but the chain of calls it creates through the enumerator).

Let pretend that we called GetObjectInRange function, we have:

PooledEnumerable with m_Enumerator = ObjectEnumerator (the one below), m_Depth = 1;
ObjectEnumerator with m_Enumerable = PooledEnumerable (the one above), m_Enumerator = SectorEnumerator (the one below), m_Map = whatever but not null
SectorEnumerator with m_Enumerable = null, m_Map = whatever (the same of ObjectEnumerator).

You use foreach to loop through PooledEnumerable and when you exit from the foreach the Dispose function of PooledEnumerable is called, and this will call Free fcuntion that is:

public void Free()
{
	if ( m_Enumerator != null )
	{
		m_InstancePool.Enqueue( this );

		m_Enumerator.Free();
		m_Enumerator = null;

		--m_Depth;
	}
}

m_Enumerator is not null so it calls the ObjectEnumerator Free function that is:

public void Free()
{
	if ( m_Map == null )
		return;

	m_InstancePool.Enqueue( this );

	m_Map = null;

	if ( m_Enumerator != null )
	{
		m_Enumerator.Free();
		m_Enumerator = null;
	}

	if ( m_Enumerable != null )
		m_Enumerable.Free();
}

m_Map is not null so it continues, m_Enumerator is not null so it calls the SectorEnumerator Free function that is:

public void Free()
{
	if ( m_Map == null )
		return;

	m_InstancePool.Enqueue( this );

	m_Map = null;

	if ( m_Enumerable != null )
		m_Enumerable.Free();
}

m_Enumerable is null so we can return back through the calls.
Ok now we are in the previous call, where m_Enumerator (of ObjectEnumerator) is nulled.
Then since m_Enumerable is not null (PooledEnumerable) its free function is called, so we are again on the top Free function, where m_Enumerator is not null
and in m_InstancePool the PooledEnumerable is enqued again and m_Enumerator Free function is called again (the ObjectEnumerator one).
So now we are in the middle Free function, m_Map is null so it istantly return to the top Free function where m_Enumerator is nulled and m_Depth decreased to 0.
But we didn't finish because the top Free function was called by the middle one previously, and after that it goes to the top one, setting again m_Enumerator to null
and decrease m_Depth to -1.

This is Ok!?!?
And why when you use IPooledEnumerable you have to again call eable.Free()?

Hope you managed to follow me...

[Smjert]

So? was my thought correct?

[arul]

You've made a lot of assumptions but actually no point. No offense here, I read your post like 4 times and still can't see what is the actual question.

IPooledEnumerable implements the IDisposable interface, which means that the resources are freed in language constructs like foreach (...) {} and using(...){}. So if you use C# properly, you'll never have to call Free() manually.

Also please realize that each enumerator class maintains it's own instance pool and internal state in private variables. So changing one instance variable (m_Map) in one enumerator doesn't and can't change another variable in another class just because the classes are chained and the variable names are same.

[Smjert]

Originally Posted by arul

You've made a lot of assumptions but actually no point. No offense here, I read your post like 4 times and still can't see what is the actual question.

IPooledEnumerable implements the IDisposable interface, which means that the resources are freed in language constructs like foreach (...) {} and using(...){}. So if you use C# properly, you'll never have to call Free() manually.

Also please realize that each enumerator class maintains it's own instance pool and internal state in private variables. So changing one instance variable (m_Map) in one enumerator doesn't and can't change another variable in another class just because the classes are chained and the variable names are same.

I think i found a bug, the question was, this sounds to you a bug?

I know everything that you said, i tried to show you how things go when you debug it, and i found that it seems that you have really strange values in instance pool.
Moreover in the official RunUO code even if the enumerator class is used with foreach or similar, sometimes you find a free, sometimes not.

Maybe i have to try to explain better all the things..

Edit: the problem is that there's a recursive call that call 2 times the PooledEnumerable free function, enqueing 2 times the PooledEnumerable.
Your example about m_Map it's not good since i never said that m_Map is nulled in SectorEnumerator since is nulled also in ObjectEnumerator ... but if you see the code both are nulled in their own function.
Finally it works with the same instances of PooledEnumerable, ObjectEnumerator and SectorEnumerator..

[arul]

No the code looks fine to me, you sure sometimes need to call the Free() function manually, see the example:

IPooledEnumerable someEable = someFunctionReturningEable;

foreach(object o in someEable) { // someEable.GetEnumerator() gets called
 // some operation
} // here the someEable.GetEnumerator() object gets disposed / Free()'d

someEable.Free(); // <-- this does nothing, since the underlying enumerator has already been Freed()'d, but who knows, it can change in the future, so we rather call it.

and compare the code with this:

foreach(object o in someFunctionReturningEable) { // someEable.GetEnumerator() gets called
 // some operation
} // here the entire 'invisible' IPooledEnumerable gets disposed / Free()'d

Here you can see why you may need to explicitly call the Free() function.

Once the Free() function had been called on the PooledEnumerable object, each subsequent call to the Free() function does effectively nothing. This is a chicken and egg question.

See the following stack representation of the problem:

PooledEnumerable.instantiante
SomeEnumerator.instantiate
SomeEnumerator.free <<-- Here's where the 'master IPooledEnumerable' gets Free()'d
PooledEnumerable.free <<-- nothing happens here

[Smjert]

Originally Posted by arul

[...] See the following stack representation of the problem:

PooledEnumerable.instantiante
SomeEnumerator.instantiate
SomeEnumerator.free <<-- Here's where the 'master IPooledEnumerable' gets Free()'d
PooledEnumerable.free <<-- nothing happens here

I will try with a diagram, to explain better what i've found, since explaining recursive calls and stack status is difficult with words ^^.

[arul]

Well, there is a problem, but not in the one you described, see:

IPooledEnumerable eable = GetObjectInRange(...);
eable.Free();

Even though the code doesn't make any sense and is useless per se, it's enqueuing the PooledEnumerable twice, which can be easily fixed by changing the way the Free() function of PooledEnumerable works:

			public void Free()
			{
				if ( m_Enumerator != null )
				{
					m_InstancePool.Enqueue( this );

					IPooledEnumerator etor = m_Enumerator;	
					m_Enumerator = null;
					etor.Free();

					--m_Depth;
				}
			}

The point being, should an API take care of foolish 3rd party usage? I don't think so, if you misuse the API, you should pay the price.

[Vorspire]

Originally Posted by arul

The point being, should an API take care of foolish 3rd party usage? I don't think so, if you misuse the API, you should pay the price.

I 100% agree with this point, but I also believe that any API should be written with as much quality as to prevent such mishaps.

It's kind of like giving a 5 year-old kid an inflatable base-ball bat with a nail stuck in the end pointing outward.

[Smjert]

I found that i followed a wrong sequence of the calls.
But i remember that strange things happens if you use multiple times the same eable with a foreach.
Anyway as you stated calling eable.Free() is a bug, and if you search in official code there's a lot of places where it's used after a foreach.

[arul]

Originally Posted by Smjert

I found that i followed a wrong sequence of the calls.
But i remember that strange things happens if you use multiple times the same eable with a foreach.
Anyway as you stated calling eable.Free() is a bug, and if you search in official code there's a lot of places where it's used after a foreach.

No, you got me wrong.

If you call the Free() function after the IPooledEnumerable had been used in a foreach statement nothing happens. You let the 'bug' surface only if you call the Free() function without ever working with the underlying enumerator - which is insane.

You must not use the same IPooledEnumerable in more than one foreach loop (why'd you need to do that anyway?), because like I already said, foreach loop disposes the enumerator, which in turn calls the Free() function of the parent PooledEnumerable. It's like closing a file handle and wondering why you can't read from it.

[Smjert]

I mean strange thing about instance pool and m_Depth.. that goes negative or goes above 5 (so a message on the console appears, telling you to use Free()).
Anyway with multiple foreach i include also the instantiation of the new eable, but using the same variabile.

[arul]

All this means that you're doing it wrong, see the already existing code for proper usage. I've never had any problem with that, considering that this API is extensively used in both scripts and core without any problems, it's your problem.

[Smjert]

I have to resume this since we found again problems with pooled enumerable.
First of all we get sometimes tons of "Warning: Make sure to call .Free() on pooled enumerables.".
But i think that this could be normal.. and i don't understand why the depth oft pooled enumerables is soo low (is 5), because it's easy that some code has a depth of 5 (a foreach that loops on a eable that calls another function that use another eable again etc).
But there's another "problem" in this system.. and is that is totally wrong to manually call Free() on pooled enumerables since the decrease of m_Depth is called twice (as i showed in the first post of this thread).
I said i was wrong since normally when the foreach block finish, it disposes the enumerator not the enumerable.

There's a lot of RunUO standard code where eable.Free() is called outside the foreach.. and it has really no use.
But there's also code where eable.Free() is called inside the foreach and this bugs the m_Depth (it goes negative).
For instance in Decorate.cs, line 954.

[arul]

Originally Posted by Smjert

I have to resume this since we found again problems with pooled enumerable.
First of all we get sometimes tons of "Warning: Make sure to call .Free() on pooled enumerables.".
But i think that this could be normal.. and i don't understand why the depth oft pooled enumerables is soo low (is 5), because it's easy that some code has a depth of 5 (a foreach that loops on a eable that calls another function that use another eable again etc).
But there's another "problem" in this system.. and is that is totally wrong to manually call Free() on pooled enumerables since the decrease of m_Depth is called twice (as i showed in the first post of this thread).
I said i was wrong since normally when the foreach block finish, it disposes the enumerator not the enumerable.

There's a lot of RunUO standard code where eable.Free() is called outside the foreach.. and it has really no use.
But there's also code where eable.Free() is called inside the foreach and this bugs the m_Depth (it goes negative).
For instance in Decorate.cs, line 954.

If you need to nest five different PooledEnumerable calls, then your code is terribly wrong. If you can, provide us with a real world example where you need to nest five different PooledEnumerables.

The code in Decorate.cs seems to be wrong, submit the bug report to the RunUO bug tracker. It's one improper usage, not to mention in a script/command that gets executed like once in the server's lifetime, so it isn't that hot.

[Smjert]

Originally Posted by arul

If you need to nest five different PooledEnumerable calls, then your code is terribly wrong. If you can, provide us with a real world example where you need to nest five different PooledEnumerables.[...]

I know, but it's possible.. or we have some wrong recursion.. we'll see ^^.

Originally Posted by arul

The code in Decorate.cs seems to be wrong, submit the bug report to the RunUO bug tracker. It's one improper usage, not to mention in a script/command that gets executed like once in the server's lifetime, so it isn't that hot.

It isn't the only place where the free() is in a foreach, there's also in BaseHouse, BaseBoat (if i remember correctly) etc..