Ayende @ Rahien

It's a girl

Async API considerations

I have a method that does a whole bunch of work, then submit an async write to the disk. The question here is how should I design it?

In general, all of the method’s work is done, and the caller can continue doing whatever they want to do with they life, all locks are released, all the data structures are valid again, etc.

However, it isn’t until the async work complete successfully that we are actually done with the work.

I could do it like this:

   1: public async Task CommitAsync();

But this implies that you have to wait for the task to complete, which isn’t true. The commit function looks like this:

   1: {
   2:     // do work
   3:     return SubmitWriteAsync();
   4: }

So by the time you have the task from the method, all the work has been done. You might want to wait of the task to ensure that the data is actually durable on disk, but in many cases, you can get away with not waiting.

I am thinking about doing it like this:

   1: public Task Commit();

To make it explicit that there isn’t any async work done that you have to wait for.

Thoughts?

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

tugberk
11/27/2013 10:27 AM by
tugberk

I think you can pass a boolean param to CommitAsync method indicating whether it should complete the task even if the actual work is not fully completed.

Steve Greatrex
11/27/2013 10:33 AM by
Steve Greatrex

Why not have 2 overloads of the method that internally do the same thing that are named to distinguish between the expected functionality?

It makes the consumers code as clear as possible in both cases

tobi
11/27/2013 10:53 AM by
tobi

You could return an object:

class { Task DurableWriteCompleted; }

That documents by name what the return value means.

Kristijan
11/27/2013 10:59 AM by
Kristijan

I think this may not be entirely true: "To make it explicit that there isn’t any async work done that you have to wait for."

When you design your method signature like that, in my opinion, you are letting developer to decide how to use your API, to wait or not to, so you aren't telling him explicitly that he doesn't have to wait for this action.

Regards

Jahmai Lay
11/27/2013 11:06 AM by
Jahmai Lay
  • The convention is if it returns Task, name it *Async.

  • If it throws, Task will have exception and if it doesn't get awaited on and cause UnobservedTaskException handler to be triggered (unless you are making commitment it never, ever throws).

  • Consider making the signature for all Async methods to be Task *Async(CancellationToken token), it's very easy to ignore the token, but it is not easy to retrofit a token because you have to review call chains to ensure tokens are passed through. Only offer overloads with no parameters (which forwards calls with CancellationToken.None) at the public/top level of the API.

  • In summary, make 2 methods. void FastCommit() and Task SafeCommitAsync(), or something like that, I am sure they are both fast and safe though, right? ;)

Ivan
11/27/2013 11:48 AM by
Ivan

I agree with Kristijan.

Fujiy
11/27/2013 01:30 PM by
Fujiy

And if you return a awaitable that completes before a write to disk and have a fluent method to wait write to disk. Like this

await CommitAsync();// don't wait disk

await CommitAsync().WaitDisk();// wait disk

Or more explicit and easily to make conditional

await CommitAsync().WaitDisk(true);// wait disk

Stephen Cleary
11/27/2013 02:31 PM by
Stephen Cleary

Asynchronous operations with truly optional joins are rare. My first reaction is to use the normal "Task CommitAsync()" signature to encourage good error handling and to keep open the option of letting the method depend on other async methods in the future.

Alternatively, you could also try to split up the two operations. E.g., "void Commit()" and "async Task FlushAsync()"; or possibly "void Complete()" and "Task Completion { get; }" (modeled after IDataflowBlock). It's hard for me to say whether these options would make sense for your type.

I think any other approach would be too confusing (nonstandard) of an API.

Paul Turner
11/27/2013 03:08 PM by
Paul Turner

In this situation I might use a custom "completion option" parameter. I first saw these in the ASP.NET Web API to control when sending a request (SendAsync) should mark the task as completed:

http://msdn.microsoft.com/en-us/library/system.net.http.httpcompletionoption(v=vs.110).aspx

In your case it could be a enum:

enum CommitCompletionOption { WriteStarted, WriteCompleted }

await CommitAsync(CommitCompletionOption.WriteCompleted);

I think it's good at being explicit in which behaviour the caller is expecting, though it trades off in being quite verbose.

markrendle
11/27/2013 03:28 PM by
markrendle

@Jahmai The UnobservedTaskException event is redundant as of .NET 4.5: http://msdn.microsoft.com/en-us/library/hh367887(v=vs.110).aspx#core

@Ayende The method should be called CommitAsync because it returns a Task, so it's a TPL async pattern, and the conventions and guidelines for that pattern say VerbAsync().

markrendle
11/27/2013 03:30 PM by
markrendle

Oh, and @tugberk: boolean flags to methods are evil and wrong and result in unreadable code.

macrohard
11/27/2013 04:34 PM by
macrohard

I don't get it... just do like the other standard Microsoft API method calls.

Task WriteToDiskAsync();

You (the implementation developer) can choose to await it or not.

Paul Turner
11/27/2013 05:54 PM by
Paul Turner

@Jahmai @markrendle The changes to UnobservedTaskException in .NET 4.5 are less to do with making it redundant than they are to do with making it "friendly by default". In our scenario here, it still provides a useful "catch all" location if you didn't care to observe the task result yourself. Arguably you should attach a handler to be dealing with those. It just doesn't spell immediate doom for your application like it previously did.

Just slow, unobserved, potential doom instead.

For this scenario, that's actually a pretty valid thing to bring up. If we have the SubmitWriteAsync() method, can it fail? If so, is there a way to recover from that failure, or is the safest thing to terminate entirely?

The Task class exists to observe the state of an asynchronous operation so we can make decisions on how to proceed when the task's state changes. If that state is important to a caller, you should expose it to them as a Task. If the state isn't important, you can run this in a fire-and-forget fashion, and it shouldn't be exposed at all.

OmariO
11/27/2013 08:30 PM by
OmariO

void Commit() and Task CommitWithFlushAsync()";

Jahmai Lay
11/28/2013 01:50 AM by
Jahmai Lay

@Paul @markrendle Paul is correct. The handler still fires, and we still treat it as a "critical error" even though it doesn't crash your application, because it is still an unhandled exception which could have unintended side-effects.

TJ
11/28/2013 02:35 AM by
TJ

You should do as Paul said, but also allow the user to register a callback that will be fired only in the event of an exception happening. Having the ability should be an option for the user meaning only register it if you care about something bad happening.

David Cucci
11/28/2013 03:47 AM by
David Cucci

I think I like Steven Cleary's API the most.

Knagis
11/28/2013 02:40 PM by
Knagis

Does .NET promise that the async method will only first yield on the first 'await'? The runtime might yield the control back to the caller before doing all the work - the async contract would not be thus violated.

Nikola Radosavljevic
12/08/2013 12:08 AM by
Nikola Radosavljevic

I think you are trying to solve the problem that doesn't exist. Having a method name end with 'Async' doesn't really imply that you have to wait for it to execute. It merely means that it will execute asynchronously. My approach was to just fire the method and forget. However, a different thing bugs me. It introduces a new warning during build process. Although I dislike it as I'm doing exactly what I want to do, I understand that maybe I'm expected to at least handle exceptions which may have happened in asynchronous task.

Comments have been closed on this topic.