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

Is it ok to have a static BaseClientService inherited service instance in a .NET 4.8 ASP.NET app? #2902

Open
wzychla opened this issue Dec 16, 2024 · 3 comments
Assignees
Labels
needs more info This issue needs more information from the customer to proceed. priority: p3 Desirable enhancement or fix. May not be included in next release. type: question Request for information or clarification. Not an issue.

Comments

@wzychla
Copy link

wzychla commented Dec 16, 2024

We integrated the AndroidPublisherService in a server-side ASP.NET 4.8 app.

Our concern is the service instance lifetime. Since the class seems to create and dispose the HttpClient internally (which, as we all know is not recommended in the long run and instead the HttpClient should be shared) it would seem that making the instance static should be recommended.

Is that so? Any specific details to consider?

I know that in case of .NET Core, a built-in HttpClient instance manager can possibly be integrated with the publisher service instance. But the question is specific to .NET 4.8.

In technical terms, is it ok to have

public class AndroidPublisherServicePort
{
    private static AndroidPublisherService _service; // shared!

    public AndroidPublisherService GetService() 
    {
		if ( _service == null )
		{
			var credential = ...
			var initializer = new BaseClientService.Initializer()
			{
				HttpClientInitializer = credential,
				ApplicationName       = ...
			};

			var service = new AndroidPublisherService( initializer );

			_service = service;
		}

		return _service;
    }
	
	public async Task<...> GetPurchaseState( ... ) 
	{
	    ...
		
		var service = this.GetService(); // shared!
		var purchase = await service.Purchases.Products.Get( .... ).ExecuteAsync();
		
		return ...
	}
}

and then anytime we need the port:

var port = new AndroidPublisherPort(); 
var purchaseState = await port.GetPurchaseState( ... )

Note that the AndroidPublisherService is static in the port class but the port class is then used in use case handlers as transient.

What I need is a clear confirmation that this is correct approach. Thanks in advance.

@wzychla wzychla added priority: p3 Desirable enhancement or fix. May not be included in next release. type: question Request for information or clarification. Not an issue. labels Dec 16, 2024
@jskeet jskeet assigned amanda-tarafa and unassigned jskeet Dec 16, 2024
@jskeet
Copy link
Collaborator

jskeet commented Dec 16, 2024

Assigning to @amanda-tarafa who has more details - but yes, we generally encourage the reuse of a single instance of each client type, unless there's a reason why you need them to be differently configured (e.g. for authentication reasons).

(I'd personally encourage the use of dependency injection to achieve this singular nature rather than making it static, but that's a somewhat different matter.)

@wzychla
Copy link
Author

wzychla commented Dec 16, 2024

Thanks, it's all clear.

@amanda-tarafa
Copy link
Contributor

We provide integration support with System.Net.Http.IHttpClientFactory through the HttpClientFromMessageHandlerFactory so that you are able to follow .NET's own recommendations.

We don't have formal examples but you can see how to use this approach in our own tests and #1775 for related clarifications. It's all a little clunky because we had to maintain backwards compatibility.

Let me know if you have issues with this approach.

Otherwise, what @jskeet said, as an alternative to using System.Net.Http.IHttpClientFactory, we recommend you share a single instance of each client type/configuration. Note that with this approach you may still encounter the DNS change problem.

@amanda-tarafa amanda-tarafa added the needs more info This issue needs more information from the customer to proceed. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info This issue needs more information from the customer to proceed. priority: p3 Desirable enhancement or fix. May not be included in next release. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

3 participants