Skip to content

Conversation

@ajcvickers
Copy link
Contributor

Fixes #29733

@ajcvickers ajcvickers requested a review from a team December 4, 2022 15:51
|| _configurationSnapshot.HasChangeTrackerConfiguration)
{
var changeTracker = ChangeTracker;
((IResettableService)changeTracker).ResetState();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're calling ResetState here just in order to set _queryTrackingBehavior back to _defaultQueryTrackingBehavior, in case we don't need to set it here again? Because it also sets all the other stuff that we'll just overwrite here below. Any way for us to just capture the default in the configuration snapshot so we can always just write it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that first, but it gets complicated because at the time we create the snapshot we don't necessarily have services available to find the default, and likewise, the ChangeTracker itself may not be available. So things broke.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I just get sad when we add more stuff to the context resetting...

|| _configurationSnapshot.HasChangeTrackerConfiguration)
{
var changeTracker = ChangeTracker;
((IResettableService)changeTracker).ResetState();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I just get sad when we add more stuff to the context resetting...

@ajcvickers
Copy link
Contributor Author

ajcvickers commented Dec 4, 2022

@roji We could adds new API here to avoid setting stuff twice. Did not seem worth the extra complexity to me. @AndriySvyryd?

(Also, we're probably going to patch this, so wanted to keep it simple for that reason.)

@AndriySvyryd
Copy link
Member

We could benchmark the impact of the resetting code. Intuitively, it doesn't seem to add a significant amount of work.

@ajcvickers ajcvickers merged commit 241538c into main Dec 8, 2022
@ajcvickers ajcvickers deleted the InsistentAragorn1204 branch December 8, 2022 10:22
wtgodbe pushed a commit that referenced this pull request Jan 4, 2023
…textOptions (#29797)

* Reset query tracking behavior correctly if set on DbContextOptions

Fixes #29733

* Reset query tracking behavior correctly if set on DbContextOptions

Port of #29766
Fixes #29733
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QueryTrackingBehavior resets to TrackAll when using DbContext pooling

4 participants