SMS optimization and cleanup

[MikeAlhayek]

@jtkech made some optimization using the CreateInstance<> you mentioned. Also did some small tweaks you also mentioned.

[jtkech]

Yes looks better, I did another review.

[jtkech]

If there is no problem to call TwilioClient.Init() concurently, at least I would define a TwilioSettings _settings field that I would check and init after the initialization, this to add a minimum of atomicity. Then you can use this _settings field elsewhere, to access the phone number for example.

private TwilioSettings _settings;

private async Task EnsureClientIsIsInitializedAsync()
{
    if (_settings is not null)
    {
        return;
    }

    var settings = (await _siteService.GetSiteSettingsAsync()).As<TwilioSettings>();
    var protector = _dataProtectionProvider.CreateProtector(SmsConstants.TwilioServiceName);

    var plainAuthToken = protector.Unprotect(settings.AuthToken);
    TwilioClient.Init(settings.AccountSID, plainAuthToken);

    _settings = settings;
}

Otherwise use for example a SemaphoreSlim as we do in many places.

[MikeAlhayek]

I don't think there is an issue of using the TwilioClient.Init() all it does just sets the username/password internally. I am not sure why we need to save the settings in a private variable here. Not sure I understand the benefit or reasoning

[jtkech]

When you set multiple fields concurrently, if we don't want to use a lock because other codes are thread safe, one way to be atomic is to have a local object of a class having all these fields, then in the concurrent code create a new instance of this class, init all fields, and at the very end update this local object in one pass (this will only update the object reference pointer).

Otherwise you may ends up with unrelated field values, one important thing is to set what you are checking at the very end. Here I used TwilioSettings but you can use a dedicated class having all the fields that you need after in the code.

This is the principle, that said I saw that you then only use PhoneNumber, not PlainAuthToken, so we could only set and check a PhoneNumber, but in the future you may need other fields.

[MikeAlhayek]

Made some more changes

[jtkech]

You don't need to enlist getters in the options, only types and then use CreateInstance() here.

[MikeAlhayek]

I thought about that. The problem is that we expose the directory in the SmsProviderOptions. someone can register an invalid types in the directory not just a Type that implements the ISmsProvider

another thing we can do is

public class SmsProviderOptions
{
    public Dictionary<string, ISmsProvider> Providers { get; } = new();
}

Then instead of the getter we can do something like serviceProvider.CreateInstance(provider.GetType())

If we store type, we would have to manage the Dictionary privately and then expose AddProvider method that would check the type before adding it to the collection.

[jtkech]

No problem to enlist types and AddSmsProvider() has a constraint on ISmsProvider.

[jtkech]

Here you can just enlist types.

[MikeAlhayek]

I made some changes let me know what you think.

Another thing we can do here is use the class name of the provider as the technical name instead of a predefined string. This will simplify things, but will be a problem if one decide to rename a provider class name.

[jtkech]

Yes looks good, I will review asap.

As you want about the key.

[jtkech]

privetly => privately

Better here to first init a variable settings, then uses it for the initialization and just after do _twilioSettings = settings, or maybe just _settings = settings as we are in the twilio provider.

Otherwise a thread may see _twilioSettings not null before it is initialized.

will review the rest asap.

[jtkech]

Okay I will review tomorrow.

In the meantime, after a quick look at the twilio code I saw that it has an inner static rest client, so I'm suspecting that it will be shared across tenants and that concurrent sendings from different tenants with different credentials may not work.

Note: We could think about enqueuing messages to be sent one after each other by a shared host level service, but each time Twilio would create a new rest client after invalidating the previous one.

So we would need a rest client per tenant and then pass it to the methods. We would need to get this rest client from twilio if it is null, without a lock if this is not resources consuming and if there is no problem if twilio creates multiple rest clients for the same credentials, otherwise use a simple lock.

Note: As we did for Redis we could think about a host level factory that would share rest clients already built for given credentials, but just in case that multiple tenants may have the same credentials.

[MikeAlhayek]

@jtkech I eliminated the use of the Twilio client and referenced from the project. Instead, I am doing the API call manually. Let me know your thoughts on the change and if you have other suggestions.

[jtkech]

Okay, I will not review all the code but LGTM to use an HttpClient.