0

I'm trying to repair some code that I cloned from a public repo. It's an async method that's missing an await operator:

public async Task<IEnumerable<JsonPatchOperation>> GetRemoveAllRelationsOperations(IBatchMigrationContext batchContext, WorkItem targetWorkItem)
{
  return targetWorkItem.Relations?.Select((r, index) => MigrationHelpers.GetRelationRemoveOperation(index));
}

I'm trying this:

public async Task<IEnumerable<JsonPatchOperation>> GetRemoveAllRelationsOperations(IBatchMigrationContext batchContext, WorkItem targetWorkItem)
{
  return await Task.Run(o => targetWorkItem.Relations?.Select((r, index) => MigrationHelpers.GetRelationRemoveOperation(index)));
}

...but I'm getting an error in the IDE:

Delegate 'Action' does not take 1 arguments

I found some similar discussions, but unfortunately none of them quite address the lambda syntax:

It appears the precompiler is interpreting the input as an Action when it should be seeing it as a Func instead. But I thought that the statement o => ... could indicate either.

I'm not familiar enough with C# to be able to work this one out. Can someone assist?

How do I tell the precompiler that I want to send a Func instead of an Action?

Johnathan Barclay
  • 18,599
  • 1
  • 22
  • 35
InteXX
  • 6,135
  • 6
  • 43
  • 80

1 Answers1

3

Well, the reason for the compilation error, is that Task.Run accepts a (non-generic) Action, which is a delegate that accepts no arguments.

You have tried to call Task.Run with a lambda accepting an argument o, so changing to this will remove the error:

Task.Run(() =>

The parentheses () denote no arguments within a lambda expression.

Having said that, wrapping a synchronous function in Task.Run is an anti-pattern.

If your method is completely synchronous, you should ideally expose it as such:

public IEnumerable<JsonPatchOperation> GetRemoveAllRelationsOperations(IBatchMigrationContext batchContext, WorkItem targetWorkItem)
{
    return targetWorkItem.Relations?
        .Select((r, index) => MigrationHelpers.GetRelationRemoveOperation(index));
}

If you cannot change the signature, for example if you are implementing an interface, then use Task.FromResult instead:

public async Task<IEnumerable<JsonPatchOperation>> GetRemoveAllRelationsOperations(IBatchMigrationContext batchContext, WorkItem targetWorkItem)
{
    return Task.FromResult(targetWorkItem.Relations?
        .Select((r, index) => MigrationHelpers.GetRelationRemoveOperation(index)));
}

This just wraps the synchronous result in a Task object, rather than forcing the lambda to run on the threadpool.

Johnathan Barclay
  • 18,599
  • 1
  • 22
  • 35
  • That works, thanks. `wrapping a synchronous function in Task.Run is an anti-pattern` Hm, interesting. What's the difference between `Task.Run` and `Task.Result`? – InteXX Jun 22 '20 at 13:20
  • @InteXX Not sure what you mean by this. Could you share your `async` / `await` chain? – Johnathan Barclay Jun 22 '20 at 13:23
  • "Async all the way down." An `async` call requires an `async` call, which requires an `async` call, etc. etc. How then to terminate that, when we've reached the end of our call chain and the last lines of code are synchronous? From your reply, it seems to be `Task.FromResult`, but I haven't investigated that yet. – InteXX Jun 22 '20 at 13:31
  • @InteXX Async starts at the bottom, which is ultimately an I/O operation, and works its way up, not the other way around. If your process doesn't involve asynchronous work, using `Task.Run` just introduces thread switching and the overhead that brings. – Johnathan Barclay Jun 22 '20 at 13:40
  • @InteXX [This](https://blog.stephencleary.com/2013/10/taskrun-etiquette-and-proper-usage.html) is a good series to read. – Johnathan Barclay Jun 22 '20 at 13:47
  • Top, bottom... I guess it depends on one's perspective. But the problem remains—given that `Task.Run()` is an anti-pattern, as you say, how then to properly terminate the chain when the final bit of our code is synchronous and can't be made otherwise? `async`/`await` must remain in order to keep the UI responsive. – InteXX Jun 22 '20 at 13:48
  • Thanks, I like Stephen's stuff. I'll have a go at that. I'm still researching this—your anti-pattern assertion has shaken me up a bit. – InteXX Jun 22 '20 at 13:50
  • @InteXX `Task.Run` should be used at the highest level possible. Keeping the UI thread free is a valid use case, but that decision should be made at UI level. – Johnathan Barclay Jun 22 '20 at 13:53
  • OK, sounds good—but I'm still not seeing how to terminate the chain while avoiding the anti-pattern. Let's take the example at hand: a `Select()` lambda. There's no such thing as `SelectAsync()`, and we have to use an `await` somewhere in that method, so the question is... where? How? Until you mentioned `Task.FromResult` just now, `Task.Run` seemed the only way. – InteXX Jun 22 '20 at 14:01
  • @InteXX `Select` is a naturally synchronous (CPU-bound) operation, which is why there is no asynchronous equivalent. Don't try to force `GetRemoveAllRelationsOperations` to be `async` when it isn't. If it transpires that synchronous code, such as `GetRemoveAllRelationsOperations`, is causing your UI thread to block, then use `Task.Run` to offload it to the threadpool. Make this decision in your UI code. – Johnathan Barclay Jun 22 '20 at 14:06
  • Ah, so it's not so much "don't do this" as it is "choose the right place to do it." Stephen seems to be saying this as well, in the article you linked. Is that it? – InteXX Jun 22 '20 at 14:09
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/216437/discussion-between-intexx-and-johnathan-barclay). – InteXX Jun 22 '20 at 14:11