Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Efcore issue 31178 #89185

Merged
merged 4 commits into from
Jul 20, 2023
Merged

Efcore issue 31178 #89185

merged 4 commits into from
Jul 20, 2023

Conversation

mapogolions
Copy link
Contributor

@mapogolions mapogolions commented Jul 19, 2023

@steveharter
This is related to dotnet/efcore#31178 and #89109
Reworked the logic a bit and made it look like what it was before this PR. This helps pass failed tests of efcore. Could you please check it

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2023
@ghost
Copy link

ghost commented Jul 19, 2023

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

This is related to dotnet/efcore#31178.
Reworked the logic a bit and made it look like what it was before. This helps pass failed tests of efcore. Could you please check it

Author: mapogolions
Assignees: -
Labels:

area-Extensions-DependencyInjection, community-contribution

Milestone: -

DependencyInjectionEventSource.Log.ServiceResolved(this, serviceIdentifier.ServiceType);
return value;
};
return new ServiceAccessor { CallSite = callSite, RealizedService = scope => value };
Copy link
Member

Choose a reason for hiding this comment

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

So the issue was that the CallSite was not captured previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I don't quite understand why this caused the error. At the moment I can't write a unit test that reproduces the crash. I just rolled back the logic as it was and tried it on ef core tests. The tests were green, at least on my machine. More research needed

Copy link
Member

Choose a reason for hiding this comment

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

I have a local repro. I'll update this PR with that soon.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't able to push to your branch - see the comments on the PR for the test.

The issue is related to swapping to an emit-based version of the service accessor which doesn't have the logic that was added to the previous lambda.

@steveharter
Copy link
Member

@mapogolions thanks for jumping on this; I was also working locally on a fix and about to reach out to you.

Please add a test that verifies the EF issue before\after - LMK if you want me to assist.

@@ -262,5 +260,11 @@ public ServiceProviderDebugView(ServiceProvider serviceProvider)
public bool Disposed => _serviceProvider.Root.Disposed;
public bool IsScope => !_serviceProvider.Root.IsRootScope;
}

private class ServiceAccessor
Copy link
Member

Choose a reason for hiding this comment

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

This should be sealed.

@steveharter
Copy link
Member

@mapogolions here's a repro that is related to the EF issue - please add this test to ServiceProviderValidationTests.cs following the existing test GetService_Throws_WhenGetServiceForScopedServiceIsCalledOnRoot:

        [Fact]
        public async void GetService_Throws_WhenGetServiceForScopedServiceIsCalledOnRoot_IL_Replacement()
        {
            // Arrange
            var serviceCollection = new ServiceCollection();
            serviceCollection.AddScoped<IBar, Bar>();
            var serviceProvider = serviceCollection.BuildServiceProvider(validateScopes: true);

            // Act + Assert
            using (var scope = serviceProvider.CreateScope())
            {
                // Switch to an emit-based version which is triggered in the background after 2 calls to GetService.
                scope.ServiceProvider.GetRequiredService(typeof(IBar));
                scope.ServiceProvider.GetRequiredService(typeof(IBar));

                // Give the background thread time to generate the emit version.
                await Task.Delay(100);

                // Ensure the emit-based version has the correct scope checks.
                var exception = Assert.Throws<InvalidOperationException>(serviceProvider.GetRequiredService<IBar>);
                Assert.Equal($"Cannot resolve scoped service '{typeof(IBar)}' from root provider.", exception.Message);
            }
        }

@steveharter
Copy link
Member

Thanks again @mapogolions

@steveharter steveharter merged commit 485e4bf into dotnet:main Jul 20, 2023
100 of 103 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-DependencyInjection community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants