Capture events in constructor configuration when using context pooling

[ajcvickers]

Fixes #19193
Fixes #17086

Notes:

• We now no longer reset context events, as per Capture events in constructor configuration when using context pooling #19193
• However, I left the code that resets options like QueryTrackingBehavior since that still seems useful, and makes this less of a break
• Added events to DbContext. These are easy and decoupled, but sync only.
• A derived DbContext can override the On... event methods. This allows async hooks.
• DbContextFactory now has a CreateDbContextAsync method such that a context can be rented from the pool and the hook applied in an async context.

[roji]

Will review and benchmark tomorrow.

[ajcvickers]

@dotnet/efteam Interested to hear feedback on the team for the second commit here. It adds async events, which are a bit non-standard compared to traditional events. Having ways to do some kind of async handling in events is becoming increasingly important when those events may be used to call other services, such as to get an access token or find a tenant ID.

I also made the non-async events align better with the async ones, but we could be completely vanilla on sync ones.

Thoughts?

[roji]

• The first commit doesn't seem to impact perf, but the second regresses RPS by 0.7% (97,560 -> 96,796). I admit I'm generally about wary about introducing more virtual extension points and logic in this extremely hot path...
• I'm curious about the real-life use cases of async OnLeased/OnReturned hooks, do we have examples? Access tokens seem to be a connection open interception thing rather than a DbContext pooling thing. And if users are doing any async operations (e.g. network I/O) at DbContext rent-time in order to get a tenant ID, then the whole point of doing DbContext pooling is probably moot perf-wise...
• As things stand, there doesn't seem to be a way to create a DbContext with async lease setting via DI - only when using PooledDbContextFactory directly from code.
• If we're introducing rent/return hooks, we could consider moving the entire config snapshotting and state reset from DbContextPool to DbContext; this would allow disabling the whole mechanism by overriding OnLeasedFromPool/OnReturnFromPool to do nothing (#24769).
• I guess I'm not clear on why we're specifically stopping to reset context events (but continuing with the rest of the state), but I probably missed a design discussion.

[roji]

I'd split the part that does foreach/await into a local method, so that in the common scenario of not having any event, we don't pay the overhead of an async state machine:

Suggested change

	var handler = LeasedFromPoolAsync;
	if (handler != null)
	{
	foreach (var func in handler.GetInvocationList())
	{
	await ((Func<DbContext, CancellationToken, Task>)func)(this, cancellationToken);
	}
	}
	return LeasedFromPoolAsync is null
	? Task.CompletedTask
	: OnLeasedFromPoolAsyncCore(cancellationToken);
	async Task OnLeasedFromPoolAsyncCore(CancellationToken cancellationToken)
	{
	var handler = LeasedFromPoolAsync;
	if (handler != null)
	{
	foreach (var func in handler.GetInvocationList())
	{
	await ((Func<DbContext, CancellationToken, Task>)func)(this, cancellationToken);
	}
	}
	}

[roji]

As above:

Suggested change

	var handler = ReturnedToPoolAsync;
	if (handler != null)
	{
	foreach (var func in handler.GetInvocationList())
	{
	await ((Func<DbContext, CancellationToken, Task>)func)(this, cancellationToken);
	}
	}
	return ReturnedToPoolAsync is null
	? Task.CompletedTask
	: OnReturnedToPoolAsyncCore(cancellationToken);
	async Task OnReturnedToPoolAsyncCore(CancellationToken cancellationToken)
	{
	var handler = ReturnedToPoolAsync;
	if (handler != null)
	{
	foreach (var func in handler.GetInvocationList())
	{
	await ((Func<DbContext, CancellationToken, Task>)func)(this, cancellationToken);
	}
	}
	}

[ajcvickers]

Thanks for the feedback, @roji. However, I was really looking for feedback from the team on the async events pattern, not the perf.

[roji]

Yeah, I know...

I think I saw discussions a long time around why async event handlers aren't a great idea - I'll try to find some resources. It feels to me that as an extensibility point, an overridable async virtual method is sufficient: it's hard to imagine multiple parts of a user application wanting to register handlers independently on pooling lifecycle events; and if this happens, the user can very easily expose an async event themselves if they wish.

One more thought is that the expectation may be for the handlers to run in parallel (i.e. Task.WhenAny), I'm not sure. Another reason I'd leave it to the user...

[AndriySvyryd]

@roji @ajcvickers New version up

• Removed async events
• Extended the configuration snapshotting to the events. LeasedFromPool gets special treatment so it's called before being reset.

[roji]

@AndriySvyryd regardless of the async event question, I think we still had some outstanding design questions around how to deal with this (see discussion around #24797). Specifically, I think it may be cleaner/better to have any events on PooledDbContextFactory (which is now public), or any other IDbContextFactory the user chooses to use.

• It's weird/non-standard for the events to stay alive across disposal of the object they're on.
• Adding the events on DbContext isn't pay-per-play - we'd be checking for them regardless of whether they're used. With the "use your own factory" approach, the extension points are naturally already present on the DbContextFactory - just override Rent and Return and execute what you want there. If you want events, you can have them there too.
  • I admit two checks on event handlers isn't likely to affect perf meaningfully, so this is a somewhat weak argument.
• Context pooling is already a somewhat advanced/high-perf feature, so I think it's reasonable to ask users to extend PooledDbContextFactory and add their logic or hooks.
• Unless I'm mistaken, for this to work, the events must be registered in the DbContext's constructor (and never from the outside, where it's impossible to know if the events have already been registered). That seems like it limits the value of events here, which are about loose coupling; the listeners would probably have to be passed via DI to the DbContext's constructor? So this is already not a trivial/easy-to-use mechanism...

Basically I'm proposing that we be more factory-oriented around advanced DbContext creation/pooling scenarios, and that we avoid adding more extension points and APIs where existing extension points already suffice.

However, I don't want to block this if I'm the only one who cares about the above.

[AndriySvyryd]

• It's weird/non-standard for the events to stay alive across disposal of the object they're on.

I would agree for non-pooled objects. However if the object is pooled then there are some events that should be preserved in similar way as configuration and services. In this PR those events are the ones set in the constructor. This doesn't seems like a surprising behavior when used and prevents a breaking change.

• Adding the events on DbContext isn't pay-per-play - we'd be checking for them regardless of whether they're used. With the "use your own factory" approach, the extension points are naturally already present on the DbContextFactory - just override Rent and Return and execute what you want there. If you want events, you can have them there too.
  I admit two checks on event handlers isn't likely to affect perf meaningfully, so this is a somewhat weak argument.

Previously you already said this isn't an issue:

The first commit doesn't seem to impact perf

• Context pooling is already a somewhat advanced/high-perf feature, so I think it's reasonable to ask users to extend PooledDbContextFactory and add their logic or hooks.

Sure, but it's still extra work for the users.

• Basically I'm proposing that we be more factory-oriented around advanced DbContext creation/pooling scenarios, and that we avoid adding more extension points and APIs where existing extension points already suffice.

The existing extension points aren't enough, we would need some non trivial changes to allow a ReturnedToPool hook in the IDbContextFactory

• Unless I'm mistaken, for this to work, the events must be registered in the DbContext's constructor (and never from the outside, where it's impossible to know if the events have already been registered). That seems like it limits the value of events here, which are about loose coupling; the listeners would probably have to be passed via DI to the DbContext's constructor? So this is already not a trivial/easy-to-use mechanism...

In the current code both ways are possible. But when adding handlers after the constructor they will only be called once before being reset.

However I do agree that their usefulness outside of the constructor is tentative. What if we remove the LeasedFromPool and ReturnedToPool events, but keep the OnLeasedFromPool and OnReturnedToPool methods that can be overridden?

[roji]

It's weird/non-standard for the events to stay alive across disposal of the object they're on.

I would agree for non-pooled objects. However if the object is pooled then there are some events that should be preserved in similar way as configuration and services.

Do we have prior art here as an example?

In this PR those events are the ones set in the constructor. This doesn't seems like a surprising behavior when used and prevents a breaking change.

Adding the events on DbContext isn't pay-per-play

Previously you already said this isn't an issue:

I indeed can't point to a noticeable perf difference, at least not today (that last bit is important). But pay-per-play is a design principle - why introduce more stuff in our hot path when another solution already exists without adding anything?

Context pooling is already a somewhat advanced/high-perf feature, so I think it's reasonable to ask users to extend PooledDbContextFactory and add their logic or hooks.

Sure, but it's still extra work for the users.

I agree we should make things easiest and provide sugar for common scenarios, but rent/return hooks for DbContext pooling doesn't seem to fit in that definition... If users want DbContext pooling and rent-return hooks (each one of these seems already rare/advanced on its own), it seems more than reasonable for them to just extend a factory class we provide, no?

The existing extension points aren't enough, we would need some non trivial changes to allow a ReturnedToPool hook in the IDbContextFactory

This is the change proposed in #24797. Do you see any particular complexity there?

In the current code both ways are possible. But when adding handlers after the constructor they will only be called once before being reset.
However I do agree that their usefulness outside of the constructor is tentative.

Adding an accessible event which should only be used from the constructor just feels to me like a big gaping pit of failure. I'm worried that the moment this gets out, people will start adding listeners on pooled DbContext instances outside the constructor. At that point either they've introduced a hard-to-track memory leak (since they keep registering the same listener again and again), or if we reset the event handlers themselves as part of lifecycle management, their listener simply disappears. It's not immediately clear as a consumer of the API how the event is managed - is it reset when the instance is returned to the pool, or not?

Stepping back, what are the downsides of the alternative proposal in #24797 (tell users to handle pooled lifecycle on the factory)? Is it just the extra of extending PooledDbContextFactory? That solution avoids all the ambiguity around how the event works, it avoids adding two public event APIs on DbContext which very few will use...

[AndriySvyryd]

Do we have prior art here as an example?

Not that I can point to specifically, but it's what prompted #19193

Unless we add something that throws whenever a user subscribes to an event in the constructor this will continue to be a pit of failure.

In this PR those events are the ones set in the constructor. This doesn't seems like a surprising behavior when used and prevents a breaking change.

Adding the events on DbContext isn't pay-per-play

Previously you already said this isn't an issue:

I indeed can't point to a noticeable perf difference, at least not today (that last bit is important). But pay-per-play is a design principle - why introduce more stuff in our hot path when another solution already exists without adding anything?

Sure, but it's still extra work for the users.

I agree we should make things easiest and provide sugar for common scenarios, but rent/return hooks for DbContext pooling doesn't seem to fit in that definition... If users want DbContext pooling and rent-return hooks (each one of these seems already rare/advanced on its own), it seems more than reasonable for them to just extend a factory class we provide, no?

The existing extension points aren't enough, we would need some non trivial changes to allow a ReturnedToPool hook in the IDbContextFactory

This is the change proposed in #24797. Do you see any particular complexity there?

#24797 doesn't add the ReturnedToPool hook, so I can't be sure about the complexity and the potential perf impact on pooling that doesn't use the hook.

In the current code both ways are possible. But when adding handlers after the constructor they will only be called once before being reset.
However I do agree that their usefulness outside of the constructor is tentative.

Adding an accessible event which should only be used from the constructor just feels to me like a big gaping pit of failure. I'm worried that the moment this gets out, people will start adding listeners on pooled DbContext instances outside the constructor. At that point either they've introduced a hard-to-track memory leak (since they keep registering the same listener again and again), or if we reset the event handlers themselves as part of lifecycle management, their listener simply disappears. It's not immediately clear as a consumer of the API how the event is managed - is it reset when the instance is returned to the pool, or not?

Stepping back, what are the downsides of the alternative proposal in #24797 (tell users to handle pooled lifecycle on the factory)? Is it just the extra of extending PooledDbContextFactory? That solution avoids all the ambiguity around how the event works, it avoids adding two public event APIs on DbContext which very few will use...

The biggest downside with going that route is the interactions story between all the Add* methods. We already have some confusion when they are used together with different scopes. If the AddDbContextFactory also starts overriding the DbContext service or vice versa it will add more confusion. We could create a new method, like AddDbContextAndFactory, but this means we'll need design and discussions.

What if we remove the LeasedFromPool and ReturnedToPool events, but keep the OnLeasedFromPool and OnReturnedToPool methods that can be overridden?

With this change there are no new public events and the existing events behave in a consistent and predictable way.

[AndriySvyryd]

@roji This is ready for the finalest review

[roji]

Thanks @AndriySvyryd, looks good.