2

I have the following method that makes a call to an async method to get an open MailKit connection:

    public async Task Process(string[] userEmails, IEmailMessage emailMessage) {
        var smtpClient = await _smtpClient.GetOpenConnection();

        _logger.Information("Breaking users into groups and sending out e-mails");
        // More Code
    }

The method GetOpenConnection looks like this:

    public async Task<IMailTransport> GetOpenConnection() {
        var policy = Policy.Handle<Exception>()
            .WaitAndRetryForeverAsync(_ => TimeSpan.FromSeconds(60),
                (exception, _) => _logger.Error(exception, "Could not establish connection"));

        return await policy.ExecuteAsync(EstablishConnection);
    }

    private async Task<IMailTransport> EstablishConnection() {
        var smtpConfiguration = _smtpConfiguration.GetConfiguration();

        _logger.Information("Get an open connection for SMTP client");

        _smtpClient.Connect(smtpConfiguration.Network.Host, smtpConfiguration.Network.Port,
            SecureSocketOptions.None, CancellationToken.None);
        var smtpClient = _smtpClient.GetSmtpClient();

        return await Task.FromResult(smtpClient);
    }

I turned off my test SMTP server (smtp4dev) and getting the connection fails as expected. Polly dutifully retries getting the connection every 60 seconds. However, when I turn the test SMTP server back on, the code does not continue where it was awaited in the Process method and I cannot figure out what I'm doing wrong.

If I convert GetOpenConnection to a synchronous method everything works properly, but, obviously, code execution is blocked until the connection is returned.

Any assistance is appreciated.

Update:

One additional item of note, is that the code executes properly if there is no error in getting the connection. If we can successfully grab the connection the first time, it will then move to the _logger line in the Process method and execute the rest of the code. It fails to move to the _logger line when we fail to grab a connection and have to retry execution.

Update 2:

Changed the Connect method to the following:

    public async Task Connect(string host, int port = 0, SecureSocketOptions options = SecureSocketOptions.Auto,
        CancellationToken cancellationToken = default) {
        await _smtpClient.ConnectAsync(host, port, options, cancellationToken);
    }

Updated the EstablishConnection to:

    private async Task<IMailTransport> EstablishConnection() {
        var smtpConfiguration = _smtpConfiguration.GetConfiguration();

        _logger.Information("Get an open connection for SMTP client");

        await _smtpClient.Connect(smtpConfiguration.Network.Host, smtpConfiguration.Network.Port,
            SecureSocketOptions.None, CancellationToken.None);

        var smtpClient = _smtpClient.GetSmtpClient();

        _logger.Information("Got an open connection for SMTP client");

        return await Task.FromResult(smtpClient);
    }

When I do this, now Polly does not appear to retry the connection at all.

Update 3:

    public void OnPostInsert(PostInsertEvent @event) {
        _logger.Information("OnPostInsert");

        _ = DispatchEvents(@event.Entity)
            .ContinueWith(x => _logger.Error(x.Exception,
                    "An error occurred while dispatching the insert event"),
                TaskContinuationOptions.OnlyOnFaulted);
    }

    private async Task DispatchEvents(object domainEntity) {
        Ensure.That(domainEntity, nameof(domainEntity)).IsNotNull();

        _logger.Information("Dispatch domain events");

        var entityBaseType = domainEntity.GetType().BaseType;

        if (entityBaseType is not { IsGenericType: true }) return;

        if (entityBaseType.GetGenericTypeDefinition() != typeof(EntityBase<>))
            return;

        if (domainEntity.GetType().GetProperty("DomainEvents")?.GetValue(domainEntity, null) is not
            IReadOnlyList<INotification> domainEvents) { return; }

        foreach (var domainEvent in domainEvents) {
            _logger.Information("Publishing Event {@DomainEvent}", domainEvent);
            await _mediator.Publish(domainEvent, CancellationToken.None);
        }
    }
DerHaifisch
  • 394
  • 1
  • 5
  • 14
  • What is the type of the application? Is it WinForms or WPF? – Theodor Zoulias Aug 17 '22 at 00:23
  • The application type is WebApi2. The code is in a library. – DerHaifisch Aug 17 '22 at 00:33
  • Is there a reason why your `EstablishConnection` method is synchronous? – ProgrammingLlama Aug 17 '22 at 00:55
  • @DiplomacyNotWar Because the ExecuteAsync method of Polly requires an asynchronous method. – DerHaifisch Aug 17 '22 at 00:58
  • I said synchronous, not asynchronous. `Polly` requires a method that returns `Task`. Decorating a method with `async` doesn't make it asynchronous, it just allows you to use `await` within it, and to directly return the type inside of `Task< >`. I am asking why your method is synchronous, and not itself asynchronous, especially when MimeKit makes asynchronous methods available. – ProgrammingLlama Aug 17 '22 at 00:59
  • @DiplomacyNotWar I see I misread your comment, but the method is asynchronous. It is asynchronous by your definition. The return type is Task and the returns Task.FromResult(smtpClient) which would return a Task, so... – DerHaifisch Aug 17 '22 at 01:04
  • It's not asynchronous because the entire thing executes on the same thread, synchronously. – ProgrammingLlama Aug 17 '22 at 01:05
  • @DiplomacyNotWar I guess, then, I don't fully understand how to write the method so that it is asynchronous. I'm just trying to return an instance of the SmtpClient I have in the MailKit adapter. – DerHaifisch Aug 17 '22 at 01:11
  • 1
    Essentially: if you make a method that looks async but isn't, you're blocking the CPU on that thread pool thread until the synchronous method you're calling completes. For IO bound work (which a connect operation is), you should call the asynchronous Connect method, because then the thread pool thread will be freed up to serve another task, and then things will resume when the IO operation completes. MailKit has a `ConnectAsync` method which you can await. Use it. – ProgrammingLlama Aug 17 '22 at 01:13
  • Note that I can't guarantee this will solve your issue, but using asynchronous stuff in unusual ways usually results in problems, so it might be the cause of yours. – ProgrammingLlama Aug 17 '22 at 01:18
  • @DiplomacyNotWar Thanks for the assistance. I made updates in my post above. Unfortunately, when I made the updates, Polly does not attempt to retry the code. – DerHaifisch Aug 17 '22 at 01:26
  • Is the method calling `Process` calling it `await Process(`, and is the method calling that method calling it like that, etc.? – ProgrammingLlama Aug 17 '22 at 01:28
  • @DiplomacyNotWar yes. await _services.Processor.Process(notifyProgramOfficerEmailConfiguration.UserEmails, emailMessage); – DerHaifisch Aug 17 '22 at 01:30
  • And all the way up the call stack it's asynchronous? – ProgrammingLlama Aug 17 '22 at 01:31
  • @DiplomacyNotWar yes. I do have a method that I have no control over that is a synchronous method because it's an implementation of an interface from NHibernate that calls a method called DispatchEvents. I call it it like this: _ = DispatchEvents(@event.Entity) .ContinueWith(x => _logger.Error(x.Exception, "An error occurred while dispatching the insert event"), TaskContinuationOptions.OnlyOnFaulted); – DerHaifisch Aug 17 '22 at 01:36
  • @DiplomacyNotWar I posted the code above. – DerHaifisch Aug 17 '22 at 01:39
  • Can you try [one of these methods](https://stackoverflow.com/a/9343733/3181933) to handle the `Task` returned by `ContinueWith` in `OnPostInsert` and see if that solves your problem? – ProgrammingLlama Aug 17 '22 at 01:41
  • 2
    Regarding the asynchronous (or not) nature of the `EstablishConnection` method, a useful distinction is between a method's contract and implementation. This method has an asynchronous contract/signature, and a synchronous implementation. Not all people agree with these semantics, but sometimes can be useful for facilitating communication. Related: [Is having a return type of Task enough to make a method run asynchronously?](https://stackoverflow.com/questions/61837259/is-having-a-return-type-of-task-enough-to-make-a-method-run-asynchronously/61837980#61837980). – Theodor Zoulias Aug 17 '22 at 01:43
  • @TheodorZoulias Thank you for the related content. I'm not sure how to resolve making this method asynchronous other than what I have done. I'm using the async ConnectionAsync of MailKit and returning a Task. when I did that, now Polly is not even attempting to retry getting the connection. I can't tell if it's because the ConnectionAsync is waiting for the connection to return and because an error hasn't been thrown it's not retrying? – DerHaifisch Aug 17 '22 at 01:53
  • @DerHaifisch my previous comment was a side-note. Regarding the main issue, I have no idea what is the cause of the problem. The async-over-sync could be relevant, but my gut instinct is that the problem is somewhere else. – Theodor Zoulias Aug 17 '22 at 01:58
  • @TheodorZoulias agreed. – DerHaifisch Aug 17 '22 at 02:01
  • I'm inclined to agree with @Theodor. Although I suggested having some kind of `await` on the `ContinueWith`, I don't think your root cause is a deadlock because you've been essentially using it as a fire and forget method. And your `.ContinueWith` should in theory be capturing any errors. I'm clutching at straws now really. Could you perhaps show us your code for `SmtpClient`? – ProgrammingLlama Aug 17 '22 at 02:01
  • @DiplomacyNotWar I agree with you. I will freely admit that I'm not a whiz with async programming, but I'm not stupid and everything that I've read says this should be working, and I have zero idea why it's not at this point. – DerHaifisch Aug 17 '22 at 02:03
  • Another side-note: the `.ContinueWith` in the "Update 3" section ([6th revision](https://stackoverflow.com/revisions/73381586/6)) violates the [CA2008](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2008 "Do not create tasks without passing a TaskScheduler") guideline. – Theodor Zoulias Aug 17 '22 at 02:12
  • 3
    @DerHaifisch [I've put together a minimal application](https://dotnetfiddle.net/z09PzG) which uses your logic but avoids the usage of smtp client. It works fine so, I assume that your problem is related to the `SmtpClient` usage. – Peter Csala Aug 17 '22 at 08:31
  • 2
    @PeterCsala I think you are correct. I think the first problem resulted from the fact that I was calling a synchronous method asynchronously which, apparently, is wrong (oops) and now I have a problem with Mailkit. I can connect using the synchronous method no problem, but cannot connect asynchronously using the exact same options because it just hangs. I may need to see if can step into the code to find out what is going on. FML. – DerHaifisch Aug 17 '22 at 10:42
  • What is this line doing `var smtpClient = _smtpClient.GetSmtpClient();` – Charlieface Aug 17 '22 at 13:16
  • It retrieves an instance of the MailKit SmtpClient. _smtpClient is an adapter and should be renamed better to clearly indicate what it is and I plan on doing that. – DerHaifisch Aug 17 '22 at 13:51

1 Answers1

1

The problem was not MailKit or Polly. As one of the commenters above indicated and also through my own testing, Polly wasn't the issue and worked as advertised. I downloaded MailKit, compiled a debug version, ripped out the release version, and added references to the new library.

After debugging, I found MailKit also worked and established a connection. While debugging, I had a random thought that maybe the problem was leaving off ConfigureAwait(false) from my async calls and, sure enough, it was.

In a sense, I created the problem for myself because I deliberately left this off of the asynchronous calls. I experienced issues with Serilog where I found I lost the ExecutionContext in async calls and, as a result, Serilog could not capture the CorrelationId and other properties added to the original thread by enrichers. Because the ExecutionContext wasn't transferred, the SecurityContext also was not transferred so bye-bye UserPrincipal. Removing ConfigureAwait(false) seemed to remove this issue.

Once I added ConfigureAwait(false) to the async calls, magic started happening. Honestly, I don't have a complete understanding of async to posit why the removal of this method caused these issues. I understand that ConfigureAwait(false) can improve performance because there is a cost to queuing a callback to synchronize the contexts and to potentially avoid any deadlocks created through this synchronization.

Now that I have nailed down the source of the problem, I just need to figure out how to transfer the ExecutionContext (SecurityContext, etc.) to flow down the async calls.

Process:

    public async Task Process(string[] userEmails, IEmailMessage emailMessage) {
        var smtpClient = await _smtpClient.GetOpenConnection().ConfigureAwait(false);

        _logger.Information("Breaking users into groups and sending out e-mails");

        // ReSharper disable once ForCanBeConvertedToForeach
        foreach (var addressGroup in userEmails.BreakArrays(_configuration.EmailGroupSize.Size))
            await _sender.SendEmailMessage(addressGroup, smtpClient, emailMessage);

        _logger.Information("Emails sent");

        await smtpClient.DisconnectAsync(true);
        smtpClient.Dispose();
    }

GetOpenConnection:

    public async Task<IMailTransport> GetOpenConnection() {
        var smtpConfiguration = _smtpConfiguration.GetConfiguration();

        var policy = Policy.Handle<Exception>()
            .WaitAndRetryForeverAsync(_ => TimeSpan.FromSeconds(60),
                (exception, _) => _logger.Error(exception, "Could not establish connection"));

        await policy.ExecuteAsync(async () => await _smtpClientAdapter.ConnectAsync(smtpConfiguration.Network.Host,
            smtpConfiguration.Network.Port,
            SecureSocketOptions.None, CancellationToken.None).ConfigureAwait(false)).ConfigureAwait(false);

        return await Task.FromResult(_smtpClientAdapter.GetSmtpClient());
    }

ConnectAsync on the MailKitSmtpClientAdapter class:

    public async Task ConnectAsync(string host, int port = 0, SecureSocketOptions options = SecureSocketOptions.Auto,
        CancellationToken cancellationToken = default) {
        await _smtpClient.ConnectAsync(host, port, options, cancellationToken).ConfigureAwait(false);
    }
DerHaifisch
  • 394
  • 1
  • 5
  • 14
  • 1
    Nice catch and summary. One tiny advice. You don't need to write this: return await Task.FromResult(_smtpClientAdapter.GetSmtpClient()); This one is enough: return _smtpClientAdapter.GetSmtpClient(); – Peter Csala Aug 17 '22 at 16:59