One-time leased blocks should not be tracked on Return

[clrjunkie]

Is there any reason too?

[dnfclas]

Hi @clrjunkie, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

[benaadams]

Looking to drop one time leased blocks in #525

[clrjunkie]

ok.. just started looking at Kestrel so I can't comment, this is something that just caught my attention as an oversight.

[halter73]

There are a lot of places in Kestrel where internal APIs don't have checks to prevent misuse.

On one hand it is scary since if one-time leased blocks were ever returned. That would break some assumptions and possibly lead to some very subtle bugs that could be hard to track down. On the other hand, if we always validated parameters in low level code like this, there would be perf consequences.

Even without #525 being merged, the code that returned the block was returnBlock.Pool?.Return(returnBlock);. For one-time leased blocks, Pool would be null, so I don't think there is a problem there even today. After #525, we don't even need the null check, so that's even better.

@clrjunkie Have you noticed Return being used incorrectly in practice?

[clrjunkie]

@halter73 Frankly speaking, I just cloned the repo and started looking at the code. I picked the MemoryPool module as a random starting point :) so I don’t know what intrinsic relationships exists with the other modules… (perhaps I will eventually) in any case this module seems well encapsulated to require a robust interface... just my 2 cents.

[halter73]

@clrjunkie I hear you. I do feel a little silly bringing up the perf impact of null checking a property. I'll grant that there likely isn't a significant perf impact for this one change even in our plaintext benchmarks.

However this isn't the first time that I've considered adding a null check for this exact property, and I've already decided against it. MemoryPoolIterator2.CopyFrom would also break if the backing block is not from a Pool, but in practice it always is from a pool. We could have added a null check for that (and a few other things) to make the CopyFrom method more robust, but I decided against because it is called with great frequency in the CopyToFast (the method we use to write common response headers) and in other places.

Instead we decided to use Debug.Assert to hopefully catch any bugs that could be introduced early without affecting perf in production. I think an assert might also be appropriate here.

I understand wanting to make the memory pool interface robust, but adding a bunch of checks like these in all the places where they could be added would have a negative perf impact. I do think that these classes should probably be made internal though, since they aren't really designed for public consumption.

[clrjunkie]

@halter73

Instead we decided to use Debug.Assert to hopefully catch any bugs that could be introduced early without affecting perf in production. I think an assert might also be appropriate here.

Good to know! (I didn't want the pull request to be considered as too "offensive" :)

I do think that these classes should probably be made internal though, since they aren't really designed for public consumption.

Why? Maybe I like ;)

[halter73]

I do think that these classes should probably be made internal though, since they aren't really designed for public consumption.

Why? Maybe I like ;)

Mostly because the design so far is dictated by the needs of Kestrel without really any consideration for a wider audience. It's all kinda hidden in the Microsoft.AspNet.Server.Kestrel.Infrastructure namespace so I'm in no rush to change it though.

The only reason the thing is called MemoryPool2 instead of MemoryPool is because there was an older pool that has since been removed from the code base. If someone sent a PR removing all the 2s, I would probably merge it (but not right now since there are so many other open PRs).

[halter73]

Oh. And don't worry. I don't offend to easily. I (almost) always like hearing other perspectives on things.

[clrjunkie]

Actually, I think leaving the null check would be better, since the decision on whether to track the block or not is an implementation detail that is not visible to the caller. The caller must always return the allocation to the Pool regardless, that way the interface can be kept simple.

[halter73]

If you call Lease with no arguments you will always get a pooled block. Perhaps we should update the doc comments to reflect this.

In the few cases that we do provide a minimum size, you should check the Pool property yourself to determine whether or not to return it (typically by doing something like returnBlock.Pool?.Return(returnBlock);). That's the contract.

[clrjunkie]

In the few cases that we do provide a minimum size, you should check the Poolproperty yourself to determine whether or not to return it (typically by doing something likereturnBlock.Pool?.Return(returnBlock);). That's the contract.

I suggest you reconsider.. this is one more thing to remember and calls for oversights possibly by other developers that would use this facility to support other / future kestrel features, especialy when it can be easly avoided with virtualy zero perf impact.

[halter73]

Alright, I'll merge it.

How about instead of checking block.Slab != null, we check block.Pool == this? Or maybe Debug.Assert(block.Pool == this) and then if (block.Pool != null) so we highlight obvious logic errors in the calling code?

I think Pool is the more obvious property to check. Less code to change if we ever have pooled memory not backed by a slab.

[clrjunkie]

The reason I tested Slab was to stay compatible with the destruction pre-condition

~MemoryPoolBlock2()
{
Debug.Assert(!_pinHandle.IsAllocated, "Ad-hoc memory block wasn't unpinned");
// Debug.Assert(Slab == null || !Slab.IsActive, "Block being garbage collected instead of returned to pool");

        if (_pinHandle.IsAllocated)
        {
            // if this is a one-time-use block, ensure that the GCHandle does not leak
            _pinHandle.Free();
        }

        if (Slab != null && Slab.IsActive)
        {
            Pool.Return(new MemoryPoolBlock2
            {
                _dataArrayPtr = _dataArrayPtr,
                Data = Data,
                Pool = Pool,
                Slab = Slab,
            });
        }
    }

Debug.Assert(block.Pool == this) won't work for one-time leases since pool is always null

return MemoryPoolBlock2.Create(
new ArraySegment(new byte[minimumSize]),
dataPtr: IntPtr.Zero,
pool: null,
slab: null);

Therefore a ref assignment would be required.

Other than that Assert followed by Pool test sounds fine.

[halter73]

Debug.Assert(block.Pool == this) should work as long as it inside the if (block.Pool != null) block.

As far as the destructor goes I think that's mostly designed to support the currently unimplemented shrinking of the memory pool. The correct thing to do to support that would be to check if (Slab != null && Slab.IsActive) like the destructor does.

[clrjunkie]

Debug.Assert(block.Pool == this)should work as long as it inside the if (block.Pool != null) block.

but this will mask one time user returns to an incorrect pool.. the interface need to be consistent about this... a bug is a bug.

[halter73]

but this will mask one time user returns to an incorrect pool.

I don't understand. The point of the assert is so that it's not masked if the user returns a block to the wrong pool. Today, it the code won't report the error in any way.

[halter73]

Are you suggesting we always throw an exception instead of using Debug.Assert?

[clrjunkie]

One-time MemoryPoolBlock2 Leases are currently created by the following code:

return MemoryPoolBlock2.Create(
                    new ArraySegment<byte>(new byte[minimumSize]),
                    dataPtr: IntPtr.Zero,
                    pool: null,
                    slab: null);

Notice that the pool parameter is assigned null.

If we add an Assert in the Return method in order to check that a block is actually returned to the right pool than the following won’t work since an Assert will be raised regardless if the caller returned the block to the right pool or not

Debug.Assert(block.Pool == this) and then if (block.Pool != null)

Changing the order effectively means that the check is by-passed for One-time MemoryPoolBlock2 Leases because currently One-time MemoryPoolBlock2 leases are not associated with any Pool.

For example, the following will always work:

var poolA = new MemoryPool2()
var poolB = new MemoryPool2()

var block = poolA.Lease(10000);
poolB.Return(block) // This is a caller bug but it won’t be Asserted

Because:

block.Pool == null // is true

and

(block.Pool != null) 
{
     Debug.Assert(block.Pool == this)
}

The fact that we are simply discarding the block in the Return method is an internal implementation detail that is of no concern to the caller and may very well change – the caller must always do the right thing which is to return the allocated Block to the right Pool.

So in order to correctly implement this we also need to store a reference to the pool from which the One-time MemoryPoolBlock2 Lease was "leased"

if (minimumSize > _blockLength)
{
     return MemoryPoolBlock2.Create(
             new ArraySegment<byte>(new byte[minimumSize]),
                dataPtr: IntPtr.Zero,
                pool: this,
                slab: null);
}

notice pool is assigned this

[halter73]

Debug.Assert(block.Pool == this) and then if (block.Pool != null)

Changing the order effectively means that the check is by-passed for One-time MemoryPoolBlock2 Leases because currently One-time MemoryPoolBlock2 leases are not associated with any Pool.

Frankly, I wasn't that concerned about catching every logic error when I suggested adding the Debug.Assert, but I didn't realize you were suggesting setting the Pool property for large blocks as well. I figured catching some logic errors was better than not catching anything.

I like the idea always setting the Pool property. It could make things nicer since CopyFrom should work even when starting from a large block. This should allow you to remove Debug.Assert(_block.Pool != null); from the CopyFrom methods. You should also be able to change our 7 usages of Pool?.Return(block) to simply be Pool.Return(block).

Still, I would double check all references to Pool to make sure there aren't any bad assumptions being made based on whether or not Pool is null.

Since always setting Pool will mean that this PR will continue to check Slab != null to determine whether or not to really return the block, I would also check Slab.IsActive for consistency with the block destructor.

[clrjunkie]

So how would you like to go about this? all changes in a new PR? separate PR for each work item?
1.remove Debug.Assert(_block.Pool != null); from the CopyFrom methods
2.change our 7 usages of Pool?.Return(block) to simply be Pool.Return(block) .
3.check Slab.IsActive for consistency with the block destructor.

[halter73]

I would just do it all in this PR.

[clrjunkie]

Ok. tomorrow (at a completely different timezone)

Any tips on the GitHub process to augment this existing PR?

[halter73]

Sounds good. You should be able to continue pushing the the existing branch you made the PR from.

[clrjunkie]

@halter73 done.

[halter73]

Can you please remove clrjunkie@5e14e55 from this PR?

_pinHandle cannot be null because it's a struct. The current code is correct. Debug.Assert(!_pinHandle.IsAllocated);, for example, is basically asserting that the _pinHandle has not been set or it's been set then freed which is exactly what we want.

If you could, also squash all your remove null-conditional operator commits, that would be great.

Thanks!

[clrjunkie]

You are correct. I'll fix.

[clrjunkie]

Done.

[halter73]

Nit: you have spaces following the semicolon on these lines.

[halter73]

Looks good other than the trailing spaces thing.

[clrjunkie]

@halter73 Any idea why this failing?

[benaadams]

Needs rebasing?

git rebase "aspnet/dev"

[benaadams]

Though you may need to switch to your aspnet/dev branch first and first do

git pull https://github.com/aspnet/KestrelHttpServer.git dev

[clrjunkie]

@benaadams Thanks, I'm on a different computer and I simply cloned my fork
My remotes are configured as:

C:\clrjunkie\KestrelHttpServer>git remote -v
origin https://github.com/clrjunkie/KestrelHttpServer.git (fetch)
origin https://github.com/clrjunkie/KestrelHttpServer.git (push)
upstream https://github.com/aspnet/KestrelHttpServer.git (fetch)
upstream https://github.com/aspnet/KestrelHttpServer.git (push)

[benaadams]

Might want to pull project.json changes out (is just whitespacing) and squash?

[halter73]

clrjunkie@5dfc268 should not be included in this PR. Instead you should use git rebase to rebase your other 4 commits on top of the current dev branch. Then you can force push your rebased branch to update this PR.

I'm happy to do this for you and close this PR, but I understand if you want to figure this out yourself.

[clrjunkie]

@halter73 @benaadams Thanks.

@halter73
I think this Ok now, please let me know if there are any problems.

BTW, there are only 6 removals null-conditional operators. The 7th was removed in commit afe944c

[halter73]

@clrjunkie Thanks!

[clrjunkie]

@halter73

No problem... It was a good exercise :)