-
Notifications
You must be signed in to change notification settings - Fork 508
Expand a few DI-related tests #201
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
Conversation
IServiceProvider services = sc.BuildServiceProvider(); | ||
|
||
McpServerTool tool = services.GetRequiredService<McpServerTool>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IServiceProvider services = sc.BuildServiceProvider(); | |
McpServerTool tool = services.GetRequiredService<McpServerTool>(); | |
IServiceProvider services = sc.BuildServiceProvider(validateScopes: true); | |
await using var scope = services.CreateAsyncScope(); | |
McpServerTool tool = scope.ServiceProvider.GetRequiredService<McpServerTool>(); |
Nit: Not a big deal since this is just a test, but resolving a scoped service without first creating a scope is bad practice, and our hosts now guard against it in dev environments. https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/9.0/hostbuilder-validation
Again, it doesn't really matter, but it's good to always show resolving scoped services from a scope. We might need to update SetupGet(x => x.Services).Returns(scope.ServiceProvider);
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see you had already merged. Nevermind. It's definitely no big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolving a scoped service without first creating a scope is bad practice
Is there a product change we need here, to ensure the IServiceProvider passed to the tool's invocation is appropriately scoped? I'm looking at #198 wondering how to repro that, but maybe we need to be creating a scope as part of a stdio server's handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to make any product changes related to scopes. At least not without deeper discussion.
Using scoped services in MCP tools is not currently supported for stdio scenarios (unless you disable validation) because there are no scopes. Without creating scopes, it's hard to see the point of scoped services anyway. You should be able to resolve request scoped services in tools invoked via MapMcp though. I'll add tests for this in my next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless you disable validation
Where is that validation coming from / why is my test above not hitting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation is coming from the host in "development" environments from here. This used to always default to false until .NET 9.
I also left a comment on #198 after making my last comment here. After further thought, I do think we should make product changes. We should create a scope per handler invocation. I assigned the issue to myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation is coming from the host in "development" environments from here. This used to always default to false until .NET 9.
Ok, so this test isn't hitting that because it's not using a host.
No description provided.