3

I'm working on a class which has a method with Action<T> parameter:

public void RegisterCallback<T> (Action<T> callback);

This works good enough.

instance.RegisterCallback<string>(Callback); //method group
instance.RegisterCallback<string>(t => {}); //lambda

Now I want to create an overload for this method which accepts async method. So that I can use it with task returning callbacks and treat them in a different way.

instance.RegisterCallback<string>(AsyncCallback); //method group with async

Where AsyncCallback is

    private Task AsyncCallback(string s)
    {
        return Task.Delay(0);
    }

The naive approach was to have a method like this one:

void RegisterCallback<T>(Func<T, Type> callback);

It has some problems though:

//1
instance.RegisterCallback<string>(async t => await Task.Delay(0));
//2
instance.RegisterCallback<string>(AsyncCallback); //method group with async

the first one gets resolved to Action<T> callback overload and the second fails to compile because of ambiguous invocation.

Well, that makes sense and I'm ok about this interface:

 void RegisterAsyncCallback<T>(Func<T, Task> callback);

However these two calls are compiled with no problems:

   instance.RegisterCallback<string>(async t => await Task.Delay(0));
   instance.RegisterAsyncCallback<string>(async t => await Task.Delay(0));

Is there a way to design this public api so that a user will only use void callbacks with one method and task returning with another.

Perhaps someone can point me to the existing api where similar problem was solved?

The full code can be found here.

Community
  • 1
  • 1
Anton Sizikov
  • 9,105
  • 1
  • 28
  • 39

2 Answers2

3

There's no good solution here; you'll either have the ambiguous call error with method overloads, or you'll have a whole new API.

Personally, I would add the Func<T, Task> overload and take the breaking change, no longer allowing method overloads. Alternatively, you can create both overloads on the new method (RegisterCallbackEx?) and mark the old one as Obsolete - this would allow older code to compile but encourage devs to change the calls.

Perhaps someone can point me to the existing api where similar problem was solved?

Well, I can give you an example where a similar problem wasn't solved. :)

Task.Factory.StartNew is commonly used to queue work to the thread pool. It's Action-based, and I believe it works with method groups.

When the .NET team wanted to add async support, they introduced a new API instead: Task.Run, which is overloaded for Action and Func<Task>.

(Note that in this case, Task.Run technically supports only a subset of StartNew uses, and StartNew was not marked Obsolete).

They considered their options, but this was the best they could come up with. Note that the following problems still exist, and developers ask questions about both of these points rather regularly here on SO:

  • Task.Run does not support method groups.
  • StartNew will compile just fine with async lambdas, but will behave in surprising ways (due to async void).

Since they were changing the .NET BCL, backwards compatibility was paramount. For a personal library, I prefer safer/cleaner APIs over strict backwards compatibility, so I would make the other choice in your situation (i.e., just add a Func<T, Task> overload as part of a major version upgrade).

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
2

Is there a way to design this public api so that a user will only use void callbacks with one method and task returning with another.

No, not really. The issue is the Action<T> delegate. It allows for a large number of various lambda expressions as you have already discovered. The one major issue that you will run into with the RegisterCallback(Action<T> callback) - if anyone uses it like this:

instance.RegisterCallback<string>(async _ => await Task.Delay(0));

They are creating an async void and this is a huge problem. You had the right idea with the Func<T, Task>. Also, I like your idea to rename the function and adding "Async" seems appropriate, RegisterAsyncCallback(Func<string, Task> callback).

David Pine
  • 23,787
  • 10
  • 79
  • 107
  • This is spot on - I forgot `async void` can be cast to `Action`. As far as I'm aware, there's no way to write a method which explicitly forbids async/non-async delegate parameters – Rob Jan 22 '16 at 13:20
  • I'm not sure what do want to know about implementation? It's just a callback(value). I need a bit more complicated error handling in the async overload. – Anton Sizikov Jan 22 '16 at 13:30
  • Update your fiddle with more code and I'll have another look. The issue that I see is `Action` will always accept `RegisterCallback(async _ => await Task.Delay(0));`. So there is no way to distinguish. – David Pine Jan 22 '16 at 13:32
  • That makes more sense now, originally you didn't provide that `IMyInterface` was actually `IMyInterface`. Like I said (and Steve Cleary agrees), add the `Func` and make the best of it. – David Pine Jan 22 '16 at 14:00