"True" async methods

[quentez]

The current async methods are just wrappers that run their synchronous counterparts on their own threads. Do you plan on refactoring this at some point so that everything uses network I/O completion ports instead of requiring threads?

(We need to maintain thousands of IMAP IDLE connections, and we're running into scalability issues because of the current threading model).

[jstedfast]

It's something I'm aware of and would like to change, but unfortunately I don't have a timeline because it's not likely to be a trivial change to make :-(

[quentez]

Thanks for the quick answer! I'll make a fork this weekend and start working on a PR of some sort to go in that direction. Thank you for your great work on this library btw, it's far above everything else when it comes to IMAP :) !

[jstedfast]

Thanks :-) MailKit started out as just a proof-of-concept to see what other APIs might be needed for MimeKit but then it turned out to be so much better than any other IMAP library that I ended up continuing to improve it.

[jstedfast]

I started an async port of the IMAP code in the 'async' branch

[quentez]

Oh wow, that's fantastic news! I had started a conversion but had to put it on hold since a threaded implementation ended up working up decently for our use case and I couldn't justify the time used to convert the whole lib.

I'd be happy to test the async implementation and give you feedback as soon as you have something that starts to work!

[naeemkhedarun]

Great to hear that! I'll also be happy to test out the async implementation once you give us a heads up its ready :-)

[naeemkhedarun]

Hey @jstedfast How did this go in the end? Is there anything I should know before trying it out and fixing bugs?

[jstedfast]

I haven't had time to work on it :(

[naeemkhedarun]

No worries I'll try to take a look at it

[naeemkhedarun]

I decided to start fresh on top of the latest commit as the code in the async branch doesn't compile nor merge well with the latest master.

Its quite difficult trying to async around the unsafe code. And the changes are creeping into MimeKit through the ICancellableStream interface changes. Is that ok?

[jstedfast]

What changes are you making to ICancellableStream? Stream's ReadAsync, WriteAsync, etc all have CancellationToken params, so they should make ICancellableStream obsolete.

[naeemkhedarun]

Great, I already had code like the below which seemed unnecessary:

if (cancellable != null) {
    await cancellable.Write (filtered, filteredIndex, filteredLength, cancellationToken);
} else {
    cancellationToken.ThrowIfCancellationRequested ();
    await Source.WriteAsync (filtered, filteredIndex, filteredLength, cancellationToken);
}

I'll remove the cancellable code paths and ThrowIfCancellationRequested checks.

[naeemkhedarun]

To make sure I'm going in the right direction, below is my plan.

• Convert IO calls to async.
• Bubble async around the solution.
• Rename converted async functions to SomethingAsync.
• To reduce breaking changes for other library users replace missing public members with Something() { SomethingAsync().Wait() or .Result; }

Hopefully the public API surface will be identical. Unfortunately some abstracts and interfaces will have to change to support both sync and async.

Is that reasonable?

[Maxwe11]

To make sure I'm going in the right direction, below is my plan.

---

Do you plan on refactoring this at some point so that everything uses network I/O completion ports instead of requiring threads?

"true" async means using I/O completion ports which currently exposed for network only via SocketAsyncEventArgs api. I've looked at code and seems current implementation of MailKit based on NetworkStream which doesn't override ReadAsync/WriteAsync and correspondingly doesn't use I/O completion ports.

[naeemkhedarun]

@Maxwe11 Both the async branch and my own are taking the Async calls on the bcl streams and surfacing them to the top of the api. Afaik this should equate to 'true' async when making calls to the network.

The old Task.Run async will be replaced with these new calls.

[jstedfast]

FWIW, the reason the async branch did not compile is because I never finished the port. The ImapStream, ImapUtils, ImapEngine, ImapCommand, etc changes should have all applied fairly cleanly.

I think I added more ImapClient and ImapFolder methods since I created the async branch, so those might not have applied and/or may have ended up missing some of the new IMailFolder interface methods.

[naeemkhedarun]

I think starting fresh has helped me get a better understanding of the codebase, I've managed to get a little further with the port.

I'll create a PR this evening so you can track progress / feedback. All commits will compile (using .Result / .Wait) even if its not fully async while we work to convert everything.

[naeemkhedarun]

You can track my progress on this PR https://github.com/jstedfast/MailKit/pull/231

If you could check out the description I would be grateful for some of your thoughts there.

[tofutim]

Any updates?

[jnm2]

From what I understand, you definitely do not want to remove the original synchronous core and wrap async I/O completion with .Result (should actually be .GetAwaiter().GetResult(). Can result in deadlocks and is inefficient. (Stephen Cleary) You need both kinds of IO all the way down.

[crozone]

Just a suggestion, over at npgsql (Postgresql adapter for EF), they had this exact same problem - how to create and maintain parallel code paths for sync and async methods, since async needs to be async all the way down.

They ended up creating another project, AsyncRewriter, which uses Roslyn to automatically convert and generate the async code at build time, based off known async counterparts for sync methods. It could be worth looking in to.

[jnm2]

One option is to pass a doAsync parameter all the way down and use if statements to determine which method to call on every single await. Or await connection.OpenAsyncMaybe(doAsync)- still very efficient in both sync and async cases:

public static class AgnosticSynchronicityExtensions
{
    private static readonly Task CompletedTask = Task.FromResult<object>(null); // Targeting pre-4.6

    public static Task OpenAsyncMaybe(this SqlConnection connection, bool doAsync)
    {
        if (doAsync) return connection.OpenAsync();
        connection.Open();
        return CompletedTask;
    }
}

[quentez]

I'm happy to see new activity on this one 😃 ! If we decide on something, I'd be happy to help for the implementation.

[ltrzesniewski]

I was quite shocked to discover how async is handled in this (primarily I/O based) library, given the quality of the rest :wink: I think you should put a big disclaimer which says that the async API is best left alone right now, as it just eats up thread pool threads for no good reason.

[jstedfast]

I've just released a version of MimeKit (1.20.0) which supports true async parsing and writing of messages/mime parts/etc.

For version 2.0, I want to make DKIM-Signature verification async as well (since IDkimKeyLocator is an interface, it's hard to add async support to that w/o breaking existing projects for 1.x, but with 2.0 I'll have more flexibility to break API/ABI).

Along with MimeKit 2.0, I'm planning to drop support for .NET 3.5 and possibly also .NET 4.0 and make MailKit async-only. I looked into trying to use a similar approach as to what I've done in MimeKit for MailKit, but it would just be an enormous amount of work.

Looking at HttpClient, all of its methods seem to be the async versions as well - it has no "sync" versions of any method that hits the network.

[jnm2]

There are polyfills that let you use async against net40 and net35. NUnit Framework uses the net40 one (for the net40 build) and I've used the net35 one.

[jstedfast]

Are you talking about Microsoft.Bcl.Async for .NET 4.0? Would that require MimeKit/MailKit nugets to have a dependency on that?

I guess I could do that, but I still think I'm dropping .NET 3.5 support in MimeKit (MailKit is >= .NET 4.0 already).

I suppose I could use Microsoft.Bcl.Async if that works for 4.0.

[jnm2]

Yes, Microsoft.Bcl.Async. I don't see any reason why upstream packages would have to know about MailKit's dependency on Microsoft.Bcl.Async. The NUnit package neither bundles an extra DLL or has a package dependency.

I'm just mentioning an option for consideration; seems like it would be rare to target <net45 and use MailKit. There are things like the TRAVERSE ecosystem which force your extensions to run on the v2 CLR, so net35 is the highest you can go (and that's where I've used AsyncBridge.Net35).

net40/net35 are also things that you could expand to support later.

[scalablecory]

Having to choose between the two, I'd rather have pure async than pure sync. So, +1 to that.

A third option is to make your parser I/O-agnostic. I've had success doing this by turning parsers into a state machine that you push buffers into, then pull results out one by one -- with the parse results being e.g. "ListResponse, StatusResponse, NeedMoreData" -- then the next layer up can feed data in either sync or async as needed and re-call the parser. Also helps make it trivially testable.

[jstedfast]

I took another stab at supporting both sync and async and this is what I came up with for SMTP, unfortunately my unit tests all fail due to AsyncTaskMethodBuilder.SetException() being called with a null value.

https://github.com/jstedfast/MailKit/commit/19ecb0dc038112aec1549eefe12cb513fad5aabc

Exception thrown in unit tests:

Did not expect an exception in Authenticate: System.ArgumentNullException: Value cannot be null.
Parameter name: exception
  at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[TResult].SetException (System.Exception exception) [0x00051] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:677 
  at MailKit.Net.Smtp.SmtpClient+<SendCommandAsync>d__62.MoveNext () [0x00243] in /Users/fejj/Projects/MailKit/MailKit/Net/Smtp/SmtpClient.cs:436 
  at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[TResult].Start[TStateMachine] (TStateMachine& stateMachine) [0x0002c] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs:471 
  at MailKit.Net.Smtp.SmtpClient.SendCommandAsync (System.String command, System.Boolean doAsync, System.Threading.CancellationToken cancellationToken) [0x0003b] in <c3ac7df2db954c55870902834ad34fa5>:0 
  at MailKit.Net.Smtp.SmtpClient+<AuthenticateAsync>d__65.MoveNext () [0x00150] in /Users/fejj/Projects/MailKit/MailKit/Net/Smtp/SmtpClient.cs:580 
--- End of stack trace from previous location where exception was thrown ---
  at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/exceptionservices/exceptionservicescommon.cs:152 
  at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Threading.Tasks.Task task) [0x00037] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/TaskAwaiter.cs:187 
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Threading.Tasks.Task task) [0x00028] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/TaskAwaiter.cs:156 
  at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd (System.Threading.Tasks.Task task) [0x00008] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/TaskAwaiter.cs:128 
  at System.Runtime.CompilerServices.TaskAwaiter.GetResult () [0x00000] in /Users/builder/data/lanes/5533/mono-mac-sdk/external/bockbuild/builds/mono-x64/mcs/class/referencesource/mscorlib/system/runtime/compilerservices/TaskAwaiter.cs:113 
  at UnitTests.Net.Smtp.SmtpClientTests+<TestBasicFunctionality>d__5.MoveNext () [0x00569] in /Users/fejj/Projects/MailKit/UnitTests/Net/Smtp/SmtpClientTests.cs:308 

Has anyone seen anything like this and know how to solve it?

async Task<SmtpResponse> SendCommandAsync (string command, bool doAsync, CancellationToken cancellationToken)
{
    var bytes = Encoding.UTF8.GetBytes (command + "\r\n");

    if (doAsync) {
        await Stream.WriteAsync (bytes, 0, bytes.Length, cancellationToken).ConfigureAwait (false);
        await Stream.FlushAsync (cancellationToken).ConfigureAwait (false);

        return await Stream.ReadResponseAsync (cancellationToken).ConfigureAwait (false);
    }

    Stream.Write (bytes, 0, bytes.Length, cancellationToken);
    Stream.Flush (cancellationToken);

    return Stream.ReadResponse (cancellationToken);
}

[jstedfast]

nevermind, figured it out... SmtpStream.WriteAsync() called itself instead of my internal WriteAsync method.

[jstedfast]

For those that want to follow along, I've created a new async branch where everything to make MailKit fully async will go before getting merged to master.

I also have an smtp-async branch which is where I ported the SMTP logic to async. I'm about to start working on porting POP3 next and that will go to a pop3-async branch until that is ready to merge into async.

[jstedfast]

POP3 has now been ported.

[jnm2]

I use if (doAsync) await task.ConfigureAwait(false); else task.AssertCompletedSynchronously(); for code that does double duty. 😃

AssertCompletedSynchronously is an extension method that throws InvalidOperationException if Task.IsCompleted is false.

[jstedfast]

The only thing I don't like about the doAsync pattern is that it clutters my list of stack frames in the debugger with a ton of compiler generated stack frames, so following it is a bit harder... 😞

[jnm2]

I know. But you'd have those for the doAsync = true path anyway.

I like @scalablecory's state machine idea though (https://github.com/jstedfast/MailKit/issues/189#issuecomment-340240535). I'll have to try it sometime. You could avoid using await and async which is a performance boost.

[jstedfast]

Using a push parser is not realistic at this point. I would need to completely rewrite all of the backends as well as MimeKit.

I went with a pull parser because that is a lot easier to abstract and use.

[jstedfast]

Making progress with IMAP. So far I've ported ImapStream.cs, ImapEngine.cs, ImapCommand.cs, ImapUtils.cs, and now ImapClient.cs... still need to port ImapFolder.cs and update the unit tests to make sure everything works.

Unfortunately, ImapFolder.cs is massive so it'll take me a while. Doesn't help that my IDE's editor is super laggy editing that file. I might have to split it up into multiple files.

[jnm2]

If you're talking about VS, I've turned massive files from molasses that freezes for five seconds every few keystrokes into a racecar by disabling ReSharper.

[jstedfast]

Yea, disabling ReSharper makes a huge difference, but that's not the IDE I use for most of my development. I normally use Visual Studio for Mac, but I'm also using a 2011 MacBook Air since I can hack while relaxing on my couch. Maybe my next laptop will be a Surface Book or something. I am in major need of a new one :)

[jstedfast]

Hoping to merge the imap-async branch into the async branch by this weekend. I've basically got stuff ported, now comes cleanup and updating the unit tests.

[jstedfast]

Ended up finishing the IMAP port a bit faster than expected and got that merged into the async branch now.

Unit tests all seem to work, but I'm a bit worried about client.IdleAsync() since the unit tests don't cover that.

I think there's at least 1 bug in all of the backends, which is that I have that Poll() method so that I can wait for data to become available on the socket, but I think the fix for that will be to make the backends only use Poll() when doAsync is false. That should be trivial to fix, though, since I only use that in a handful of places.

Will look into that as well as testing IDLE tomorrow.

[alex-becker-startp]

Hi Jeff, the old async methods required reconnection when cancelled. will this new async implementation remove this limitation?

[jstedfast]

No - there's no way around needing to reconnect.

[jstedfast]

All of my async patches have now been merged to master.

[jstedfast]

hmmm, forgot about getting the .NET 4.0 project to build. Added Microsoft.Bcl.Async, but .NET 4.0 project still won't build saying no such methods: ConfigureAwait() and GetAwaiter().

[jnm2]

Oh! I was wrong earlier. NUnit Framework doesn't reference Bcl.Async; only the tests do. NUnit Framework does everything via reflection. So async on net40 is not something you'd likely get much ROI for.

[jstedfast]

Should I just kill .NET 4.0 support? Honestly, at this point, I am thinking anyone who wants to continue using .NET 4.0 could just stick to MimeKit & MailKit 1.x. They're pretty solid.

[jnm2]

The only time I can imagine someone actually needing net40 support is developing for Windows XP/Server 2003, and at that point you're almost contributing to the poor practice of running outdated and unpatched runtimes. Given recent discussion I think NUnit is very likely to drop net40.

[jstedfast]

FWIW, MimeKit and MailKit 2.0 have both been released which support true async.