danzuep / MailKitSimplified

Send and receive emails easily, fluently, with one line of code for each operation.
MIT License
61 stars 8 forks source link

Monitor multiple boxes threads issue #63

Open Sylar-A opened 1 month ago

Sylar-A commented 1 month ago

Hi, @danzuep!

At first, thanks a lot for the monitoring multiple boxes feature! At second, there is some issue with it. I have registered 7 FolderMonitors in appsettings and using IMailFolderMonitorFactory.MonitorAllMailboxesAsync method to monitor them. So, for example (and not only this example), when i get 2 mail in 1 mailbox same time, it's thows exceptions:

MailKitSimplified.Receiver.Services.MailFolderMonitor|IMAP client is being accessed by multiple threads. System.InvalidOperationException: The ImapClient is currently busy processing a command in another thread. Lock the SyncRoot property to properly synchronize your threads.
   at MailKit.Net.Imap.ImapEngine.PopNextCommand()
   at MailKit.Net.Imap.ImapEngine.IterateAsync()
   at MailKit.Net.Imap.ImapEngine.RunAsync(ImapCommand ic)
   at MailKit.Net.Imap.ImapClient.NoOpAsync(CancellationToken cancellationToken)
   at MailKitSimplified.Receiver.Services.MailFolderMonitor.ProcessMessagesArrivedAsync(Boolean firstConnection, CancellationToken cancellationToken)
   at MailKitSimplified.Receiver.Services.MailFolderMonitor.WaitForNewMessagesAsync(CancellationToken cancellationToken)
MailKitSimplified.Receiver.Services.MailFolderMonitor|Error occurred processing arrival queue item #MailKit.MessageSummary. System.InvalidOperationException: The ImapClient is currently busy processing a command in another thread. Lock the SyncRoot property to properly synchronize your threads.
   at MailKit.Net.Imap.ImapEngine.PopNextCommand()
   at MailKit.Net.Imap.ImapEngine.IterateAsync()
   at MailKit.Net.Imap.ImapEngine.RunAsync(ImapCommand ic)
   at MailKit.Net.Imap.ImapFolder.GetBodyPartAsync(UniqueId uid, String partSpecifier, CancellationToken cancellationToken, ITransferProgress progress)
   at MailKitSimplified.Receiver.Extensions.MessageSummaryExtensions.GetBodyTextAsync(IMessageSummary messageSummary, CancellationToken cancellationToken)
   at MailKitSimplified.Receiver.Services.MailFolderMonitor.ProcessArrivalQueueAsync(Func`2 messageArrivalMethod, CancellationToken cancellationToken)

At third, there is example with worker which is Singleton and have injected from constructor dependency on IImapReceiver, which is Transient. So, it won't be transient, WorkerService will control it's lifetime, and it would be Singleton. There is recomendation from Microcoft to do it rigth way.

Exceptions throws every day several times😥 Please, help, Daniel🙏

danzuep commented 1 month ago

Hi @Sylar-A, glad you found the FolderMonitor feature as I'm not sure I'd documented it fully 😊. I ran it in my test environment but the project I initially wanted to use it for has been and gone, so I haven't actually used it in a real-world setting yet. It sounds like you've figured out the fix for this issue though, so well done! Please make a fork of the project, test your proposed changes on your machine, then make a pull request to merge your changes into the main branch. Then your name will appear on the main page as a contributor 🙂

Sylar-A commented 1 month ago

Hi, @danzuep! I have found a fix for another issue in sample code, but it's not fix my issue with multiple threads😞

danzuep commented 1 month ago

As a short-term workaround you could try using the ImapReceiverFactory instead. If that works then using the copy function could be the solution.

Sylar-A commented 1 month ago

Hi, @danzuep!

If i understand you correct, it must be a BackgroundService with while loop and with some timeout? For example:

    private async Task DoWork(CancellationToken cancellationToken)
    {
        while (!cancellationToken.IsCancellationRequested)
        {
            await Task.Delay(TimeSpan.FromMinutes(1), cancellationToken);

            foreach (var receiver in _imapReceiverFactory.GetAllImapReceivers())
            {
                var messageSummaries = await receiver.ReadMail.GetMessageSummariesAsync(cancellationToken);

                foreach (var messageSummary in messageSummaries)
                {
                    // Do something...
                }
            }
        }
    }

But i didn't realize how to use the copy method as you suggested.

danzuep commented 1 month ago

The method copy is used to ensure each receiver has a unique IMAP client, but it's used inside the receiver factory and the idle client, so that should be enough. You could try adding Task.WhenAll with mailbox monitors to your code above, or try modifying the monitor factory directly to find the source of the error. Enable debug logging to see further into the stack, breakpoints are good too but harder to follow with all the multi-threading. I won't be able to do this myself for another week or so, much better for both of us if you can find the issue before then 🙂

Sylar-A commented 1 month ago

Oh, ok, Daniel, thanks for parting words, i'll try to hard debug it😅

danzuep commented 1 month ago

Looking back at the error message, it says the "IMAP client is being accessed by multiple threads." The IMAP client isn't being passed through when the MailFolderMonitor is constructed though. Then I saw the second part that says "GetBodyTextAsync(IMessageSummary)". The short term fix then is not to use messageSummary.Folder. Hopefully that helps too with a more targeted search 🙂

Sylar-A commented 1 month ago

Hi, @danzuep!

I have found another issue with IImapReceiverFactory. When it's creates new receivers and all of them have same ImapHost, it's takes only one mail (in my case one same mail seven times instead of 7 different mails). When I debugging it, the problem was in IMemoryCache extension method GetOrCreate (from Microsoft). Because of all mails have same ImapHost, it wasn't creates new item and tooks the same item 7 times by the key. It was here.

danzuep commented 1 month ago

Wow, good find! _memoryCache can be removed, it doesn't add much in terms of functionality or speed. Are you happy to add it to a pull request? If so, please submit each fix as a separate pull request if possible, but all good if not.

Sylar-A commented 1 month ago

Ok, Daniel. I could do it later.

danzuep commented 3 weeks ago

Hey @Sylar-A, is the issue you found fixed?

You also mentioned changing to scoped services, but I don't see those in the fork on your profile. I've done that for the Sender, but I guess I never got around to it for the Receiver so I should get on to it.

Sylar-A commented 3 weeks ago

Hi, @danzuep! Saddenly no🥲 I cant reproduce it in tests. I've created test email address and sent many messages same time and it worked perfectly. But it still crashing in production every day( Maybe the reason is that there is only one test email address and i must test it with many. I dont know...

Scoped services would be great! But you have to remember that if you inject scoped (or transient) service from background servise (singleton) constructor, background service will create it only once.

danzuep commented 3 weeks ago

The answer to this issue you've found is to not use use the messageSummary that comes from the MailFolderMonitor or the messageSummary.Folder. I've added a MailFolderCache function you can use from the ImapReceiver, so use the UniqueId and the GetMailFolderAsync() function instead of using the MessageSummary directly. One possible solution would be for me to just return the UniqueId instead of returning the MessageSummary.

danzuep commented 3 weeks ago

Here's a sample using the latest pre-release:

        using var scope = _serviceScopeFactory.CreateScope();
        var mailFolderClient = scope.ServiceProvider.GetRequiredService<IMailFolderClient>();
        var mailFolderMonitorFactory = scope.ServiceProvider.GetRequiredService<IMailFolderMonitorFactory>();
        async Task UniqueIdArrivedAsync(IMessageSummary messageSummary) =>
            await mailFolderClient.MoveToAsync(messageSummary, destinationFolderFullName, cancellationToken);
        await mailFolderMonitorFactory.MonitorAllMailboxesAsync(UniqueIdArrivedAsync, cancellationToken);
danzuep commented 3 weeks ago

Please test the changes and let me know how it goes.

Sylar-A commented 3 weeks ago

Hi, @danzuep! I cant find the latest pre-release as you mentioned in nuget. There is only previous version. I've opened it with dotPeek and there aren't your last changes. Maybe it's GitHub issue, because there is only one rc tag on 2.10.0 too. image

danzuep commented 3 weeks ago

Ah. it seems that GitVersion_NuGetVersion didn't update, even though the semantic version is correct. Weird, that used to work. I re-ran it with a higher version and it published this time.

Sylar-A commented 3 weeks ago

Hi, @danzuep! I've found some issues with new code🥲

  1. When i try to call await mailFolderClient.AddFlagsAsync([messageSummary.UniqueId], MessageFlags.Seen); it throws an exception:

    MailKit.Security.AuthenticationException: invalid command
    at MailKit.Net.Imap.ImapClient.AuthenticateAsync(Encoding encoding, ICredentials credentials, CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.ImapReceiver.AuthenticateImapClientAsync(CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.ImapReceiver.ConnectAuthenticatedImapClientAsync(CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.ImapReceiver.ConnectMailFolderAsync(CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.MailFolderClient.ConnectMailFolderAsync(IMailFolder mailFolder, Boolean enableWrite, CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.MailFolderClient.ConnectAsync(Boolean enableWrite, CancellationToken cancellationToken)
    at MailKitSimplified.Receiver.Services.MailFolderClient.AddFlagsAsync(IEnumerable`1 uniqueIds, MessageFlags messageFlags, Boolean silent, CancellationToken cancellationToken)

    Somewhere here (deeper in MailKit): image

  2. When i try to call await mailFolderClient.MoveToAsync(messageSummary, _archiveFolderName); it throws an NullReferenceException with _mailFolderCache because of mailFolder is null:

    System.NullReferenceException: Object reference not set to an instance of an object.
    at MailKitSimplified.Receiver.Services.MailFolderCache.CacheMailFolder(IMailFolder mailFolder)
    at MailKitSimplified.Receiver.Services.MailFolderCache.GetMailFolderAsync(IImapReceiver imapReceiver, String mailFolderFullName, CancellationToken cancellationToken)

    image

danzuep commented 2 weeks ago

It sounds like the mail folder name is not found. What do the protocol logs say? Do the folder names match?

Sylar-A commented 2 weeks ago

ProtocolLog is empty. I didn't change the folder name. It's same name that i pass to await messageSummary.Folder.GetSubfolderAsync(_archiveFolderName);.

danzuep commented 1 week ago

Some of the issues you mentioned are fixed in the latest pre-release, try updating. The main thing I haven't tried is creating folders, but the other ones should work now. The root of the issue was mistakenly using the IMAP client directly without connecting or authenticating it first.

Sylar-A commented 1 week ago

Hi, Daniel! Thank you for fixes! But I still can't see new rc version in nuget packages🥲 Maybe you'll release a new version after all fixes?

danzuep commented 1 week ago

Looks like it was there when you commented, but just in case I've made a new one. Please do a Pull Request if you notice any issues.

Sylar-A commented 1 week ago

Hi, @danzuep! Second point of comment was fixed, thanks! But first is still throws and now on both mailFolderClient.AddFlagsAsync() and mailFolderClient.MoveToAsync() methods( Maybe the problem is in empty ImapCredentials here? It's filled in appsettings.json file. image

danzuep commented 1 week ago

It seems it's a problem with your authentication settings, but the issue isn't clear from the error message (MailKit.Security.AuthenticationException: invalid command). What's the ProtocolLog saying?

Sylar-A commented 1 week ago

It says

Connected to imaps://imap.mail.ru:993/
S: * OK IMAP4 ready
C: V00000000 CAPABILITY
S: * CAPABILITY IMAP4rev1 ID XLIST UIDPLUS UNSELECT MOVE LIST-STATUS SASL-IR AUTH=PLAIN AUTH=XOAUTH2
S: V00000000 OK CAPABILITY completed
C: V00000001 AUTHENTICATE PLAIN ********
S: V00000001 NO [AUTHENTICATIONFAILED] NEOBHODIM parol prilozheniya https://help.mail.ru/mail/security/protection/external / Application password is REQUIRED
C: V00000002 LOGIN "" ""
S: V00000002 BAD invalid command
danzuep commented 1 week ago

"Application password is REQUIRED" (Gmail has the same requirement). The capability line includes AUTH=XOAUTH2, if you you just want to get it working, you can use imapReceiver.RemoveAuthenticationMechanism("XOAUTH2");.

Authenticating via a SASL mechanism may be a multi-step process. see href="http://www.mimekit.net/docs/html/T_MailKit_Security_SaslMechanism.htm" seealso href="http://www.mimekit.net/docs/html/T_MailKit_Security_SaslMechanismOAuth2.htm"

Sylar-A commented 1 week ago

Hi, @danzuep! The error is not in XOAUTH2 mechanism, it's in PLAIN. In this image you can see that credentials are not empty image In this image it's empty (when i use mailFolderClient methods AddFlagsAsync and MoveToAsync) image log:

Connected to imaps://imap.mail.ru:993/
S: * OK IMAP4 ready
C: V00000000 CAPABILITY
S: * CAPABILITY IMAP4rev1 ID XLIST UIDPLUS UNSELECT MOVE LIST-STATUS SASL-IR AUTH=PLAIN AUTH=XOAUTH2
S: V00000000 OK CAPABILITY completed