Azure / azure-notificationhubs-dotnet

.NET SDK for Azure Notification Hubs
MIT License
70 stars 122 forks source link

Azure Notification Hub static instance or recreate #124

Closed Zapnologica closed 3 years ago

Zapnologica commented 4 years ago

Good day, I made a SO question about this, but I have not any official MS response.

SO

What is the correct way to use the c# Azure Notification Hub Client?

We are using this in a very high usage server sending millions of pushes a day.

I would like to know if we should recreate the NotificationHubClient every time or if we should keep a static/singleton instance of it and then reuse that each time?

Currently, we recreate it every time we send a push notification. However, I know from personal experience we have had issues with the .net HTTP client and it not releasing tcp sockets fast enough. I was worried that this library could start having similar issues.

tiaan-lg commented 4 years ago

I am also experiencing issues with using the hub client whereby it is exhausting sockets, what is the best practice for creating the and reusing the NotificationHubClient

tiaan-lg commented 4 years ago

For anybody struggling with this, we solved it by passing in a MessageHandler from the IHttpMessageHandlerFactory this prevents it from recreating the HTTP client and starving sockets

Example code where _httpMessageHandler is a static property

// if handler is not created, then create it
if (_httpMessageHandler == null)
{
    var serviceProvider = new ServiceCollection().AddHttpClient().BuildServiceProvider();

    var httpClientFactory = serviceProvider.GetService<IHttpMessageHandlerFactory>();

    _httpMessageHandler = httpClientFactory.CreateHandler();
}

var hub = new NotificationHubClient({{Connection String}}, {{Hub Path}}, new NotificationHubClientSettings
{
    MessageHandler = _httpMessageHandler
});
mpodwysocki commented 3 years ago

@tiaan-lg yes, that would be a recommended approach. Please reopen this issue if this is still a problem.

shabirjan commented 3 years ago

@mpodwysocki Can you please clarify if this approach is correct or not, we are adding the following code in the Constructor of our Controller.

private NotificationHubClient _nhClient;
public NotificationsController(CMLogger logger,
                                       IConfiguration config, IHttpMessageHandlerFactory httpMessageHandler)
        {
            _logger = logger;
            _config = config;
            _nhClient = new NotificationHubClient(_config["NotificationsHub:ConnectionString"],
             _config["NotificationsHub:HubName"],
             new NotificationHubClientSettings
             {
                 OperationTimeout = TimeSpan.FromSeconds(10),
                 MessageHandler = httpMessageHandler.CreateHandler()
             })
        }
dzoech commented 3 years ago

@tiaan-lg @mpodwysocki

Example code where _httpMessageHandler is a static property

Wouldn't this static HttpMessageHandler result in an also static HttpClientHandler for the whole lifetime of the application and therefore, might cause issues with DNS changes?

If I understand the inner workings of DefaultHttpClientFactory (which implements IHttpMessageHandlerFactory) correctly, calling CreateHandler() everytime a NotificationHubClient with scoped lifetime is created, should reuse the same handler pipeline. At least withing the lifetime of ActiveHandlerTrackingEntry. This reusing should mitigate the problem of socket exhaustion.

Please correct me if I understood something wrong.