Is McpClientFactory's caching useful?

[stephentoub]

Most of the complexity in McpClientFactory, including it being instantiable, tracking clients, having a callback-based creation mechanism, etc., all seems to stem from a desire to be able to cache IMcpClient instances inside of the factory for subsequent use.

Is that valuable? This factory isn't thread-safe, so presumably an individual caller is constructing the factory and then using it in isolation; what scenarios would drive someone to go back to the factory each time it needs a client for a server, but still holding onto the factory instance, rather than just holding on to the client that it needs for that server?

Getting rid of that capability, and just pushing client instance management to the host, would simplify the factory and make it simpler to use, e.g. just a static McpClientFactory.Create method that also has a simpler signature.

cc: @eiriktsarpalis, @PederHP

[PederHP]

I think it's an abstraction it would be best to get rid of. The session scope abstraction that was introduced at one point should really have told me that it was overdue to get rid of the factory that is as you say spread out across multiple concerns in a not-so-great way. Connections being stateful and stdio clients being responsible for the lifetime of a server means some kind of lifecycle management is probably warranted. But I agree fully with your points. Sent from Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Stephen Toub ***@***.***> Sent: Thursday, March 13, 2025 11:36:01 PM To: PederHP/mcpdotnet ***@***.***> Cc: Peder Holdgaard Pedersen ***@***.***>; Mention ***@***.***> Subject: [PederHP/mcpdotnet] Is McpClientFactory's caching useful? (Issue #76) Most of the complexity in McpClientFactory, including it being instantiable, tracking clients, having a callback-based creation mechanism, etc., all seems to stem from a desire to be able to cache IMcpClient instances inside of the factory for subsequent use. Is that valuable? This factory isn't thread-safe, so presumably an individual caller is constructing the factory and then using it in isolation; what scenarios would drive someone to go back to the factory each time it needs a client for a server, but still holding onto the factory instance, rather than just holding on to the client that it needs for that server? Getting rid of that capability, and just pushing client instance management to the host, would simplify the factory and make it simpler to use, e.g. just a static McpClientFactory.Create method that also has a simpler signature. cc: @eiriktsarpalis<https://github.com/eiriktsarpalis>, @PederHP<https://github.com/PederHP> — Reply to this email directly, view it on GitHub<#76>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A6NR7FJ352O34IXD6TQWUCD2UIB5DAVCNFSM6AAAAABY7MNY36VHI2DSMVQWIX3LMV43ASLTON2WKOZSHEYTQNJRHA4DOMA>. You are receiving this because you were mentioned.Message ID: ***@***.***> [stephentoub]stephentoub created an issue (PederHP/mcpdotnet#76)<#76> Most of the complexity in McpClientFactory, including it being instantiable, tracking clients, having a callback-based creation mechanism, etc., all seems to stem from a desire to be able to cache IMcpClient instances inside of the factory for subsequent use. Is that valuable? This factory isn't thread-safe, so presumably an individual caller is constructing the factory and then using it in isolation; what scenarios would drive someone to go back to the factory each time it needs a client for a server, but still holding onto the factory instance, rather than just holding on to the client that it needs for that server? Getting rid of that capability, and just pushing client instance management to the host, would simplify the factory and make it simpler to use, e.g. just a static McpClientFactory.Create method that also has a simpler signature. cc: @eiriktsarpalis<https://github.com/eiriktsarpalis>, @PederHP<https://github.com/PederHP> — Reply to this email directly, view it on GitHub<#76>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A6NR7FJ352O34IXD6TQWUCD2UIB5DAVCNFSM6AAAAABY7MNY36VHI2DSMVQWIX3LMV43ASLTON2WKOZSHEYTQNJRHA4DOMA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[jtschuster]

One positive of the McpClientFactory being an object was being able to add it in DI. Creating an IMcpClient is async so I had to either create my own factory class or add the parameters to DI. Unless I'm missing something and there's an easier workaround.

[stephentoub]

add the parameters to DI

Is there a meaningful difference between that and having the McpClientFactory in DI?

Creating an IMcpClient is async

Creation on the McpClientFactory will be async regardless of whether CreateAsync is an instance method or not, right? Can you clarify how this impacts things?

[jtschuster]

add the parameters to DI

Is there a meaningful difference between that and having the McpClientFactory in DI?

I suppose there's no real meaningful difference, just an ergonomics thing.

Creating an IMcpClient is async

Creation on the McpClientFactory will be async regardless of whether CreateAsync is an instance method or not, right? Can you clarify how this impacts things?

Right, I just meant since the creation is async I wouldn't be able to add an IMcpServer to DI directly and would need to add the parameters or my own factory class instead.

Thinking through this more I see why the static factory makes sense now.