0

I just want a sanity check on this, because I have little experience writing asynchronous functions.

I am subclassing an abstract class and need to override a function with the following signature:

public abstract Task WriteResponseBodyAsync(OutputFormatterWriteContext context);

The function must return task, and my override does not use any asynchronous functions, so to return Task I use Task.Run. Is this the correct usage?

public class FhirJsonFormatter : TextOutputFormatter
{    
    public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding)
    {
        Hl7.Fhir.Serialization.FhirJsonSerializer ser = new Hl7.Fhir.Serialization.FhirJsonSerializer();

        using (System.IO.StreamWriter sw = new System.IO.StreamWriter(context.HttpContext.Response.Body))
        {
            using (Newtonsoft.Json.JsonWriter jw= new Newtonsoft.Json.JsonTextWriter(sw))
            {
                return Task.Run(() => { ser.Serialize((Hl7.Fhir.Model.Base)context.Object, jw); });
            }
        }
    }
}
plr108
  • 1,201
  • 11
  • 16
Jeremy
  • 44,950
  • 68
  • 206
  • 332
  • 2
    Use Task.FromResult. That's the best I can do rn :) – Joelius Jan 22 '20 at 16:58
  • Or `Task.CompletedTask` if you have no return value besides `Task` – JSteward Jan 22 '20 at 16:58
  • 3
    The main issue here is that the objects may be disposed before the task gets a chance to run. – Yacoub Massad Jan 22 '20 at 17:02
  • It's usually a bad idea to try to make things async by wrapping them in `Task.Run`; this has overhead that may be detrimental. The caller may actually not want the task to run in a new thread. Make your code not break if the caller wants to call it inside a new thread, but let the caller make that decision. – Dour High Arch Jan 22 '20 at 17:14
  • First check to see if there's a `SerializeAsync` you can use instead. If not then it depends on if you need to not block on the IO. To not block use `await Task.Run`, otherwise `Task.CompletedTask` will work. – juharr Jan 22 '20 at 17:14
  • The Serializer object has no async methods. I'm just not sure how to determine if I should wrap the entire contents of the method in a Task.Run instead or just use Task.Completed – Jeremy Jan 22 '20 at 17:16
  • 1
    Is `FhirJsonSerializer` code that you control? If so maybe think about adding async methods to it. It's best to do async code all the way from the top to the bottom rather than wrapping in `Task.Run` or just throwing out a `Task.CompletedTask`, if possible. – juharr Jan 22 '20 at 17:17
  • No, FhirJsonSerializer is contained in a 3rd party library. Thanks for the suggestions – Jeremy Jan 22 '20 at 17:31

1 Answers1

1

Task.Run will run the code on another thread, which is probably not what you want.

Why not just return Task.CompletedTask:

public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding)
{
    Hl7.Fhir.Serialization.FhirJsonSerializer ser = new Hl7.Fhir.Serialization.FhirJsonSerializer();

    using (System.IO.StreamWriter sw = new System.IO.StreamWriter(context.HttpContext.Response.Body))
    {
        using (Newtonsoft.Json.JsonWriter jw= new Newtonsoft.Json.JsonTextWriter(sw))
        {
            ser.Serialize((Hl7.Fhir.Model.Base)context.Object, jw);
        }
    }
    return Task.CompletedTask;
}

Also with your example, the method may return before ser.Serialize has finished, causing jw, sw and maybe even context to be disposed.

Johnathan Barclay
  • 18,599
  • 1
  • 22
  • 35
  • Should I have wrapped all the code inside the method inside a Task.Run instead? – Jeremy Jan 22 '20 at 17:13
  • The downside here is that this blocks the current thread while the IO runs. That may or may not be a factor. – juharr Jan 22 '20 at 17:14
  • _Task.Run will run the code on another thread_ [not necessarily](https://stackoverflow.com/questions/13429129/task-vs-thread-differences) – Joelius Jan 22 '20 at 19:15