Thread.Sleep or Task.Delay in a synchronous method with both a sync and async caller

Question

I have a synchronous method:

public void DoStuff() {  
    DoThings(); 
    GraphClient.SetExtendedProperty(user, propertyName, value);     // this method occasionally throws an exception
    DoOtherThings();
}

Call3rdPartyMethod makes a REST API call using the Azure Ad Graph Client API which throws an exception if I try to set the value of an extended property on Active Directory and it isn't found. This usually happens a new user is added to the directory and the Extension Properties feature hasn't extended the user schema before I want to set the values (it seems to take a few seconds).

I replaced the SetExtendedProperty call with my own wrapper containing the call in a busy-wait loop thus:

public void TrySetProperty(GraphObject user, string propertyName, string value)
{

   var exceptions = new List<Exception>();

   for (int retry = 0; retry < 5; retry++)
   {
      try
      { 
          if (retry > 0)
              Thread.Sleep(1000);
              GraphClient.SetExtendedProperty(user, propertyName, value);
      }
      catch (Exception ex)
      { 
          exceptions.Add(ex);
      }
   }

   throw new AggregateException(exceptions);
  }
}

The issue is that I want to be able to call TrySetProperty from both a synchronous and asynchronous method:

public void DoStuff() {  
    DoThings(); 
    TrySetProperty(user, propertyName, value);
    DoOtherThings();
}

public Task DoOtherStuffAsync() {
    await DoAsyncThings();
    TrySetProperty(user, propertyName, value);
    await DoOtherAsyncThings();
}

I can't change SetExtendedProperty to be asynchronous, and I'm concerned I shouldn't be using Thread.Sleep if i'm calling it from an asynchronous method - rather Task.Delay(). Can anyone advise?

Don't ever mix blocking code with async await. It's deadlock waiting to happen. I'd suggest writing all your methods in the async style with no blocking code, then using [this technique](http://stackoverflow.com/a/5097066/14357) to afford yourself a synchronous call to an asynchronous method. — spender, Feb 13 '17 at 12:11
@Botonomous the `retry` is there to avoid a delay at the first try, but insert a delay before each following try. — René Vogt, Feb 13 '17 at 12:11
deleted my answer, because @spender is right, it's not a good suggestion to mix async and sync methods this way. — René Vogt, Feb 13 '17 at 12:14
I've had to use the `AsyncHelpers` (in the answer pointed to in my previous comment) once in production and, although it worked, TBH it didn't sit nicely. Do you **really** need to go synchronous? — spender, Feb 13 '17 at 12:16
I may have just found my solution to this. There is an ExecuteSingleAsync().Result example on GetObjectById here: https://www.simple-talk.com/cloud/security-and-compliance/azure-active-directory-part-6-schema-extensions/. I guess I can now write two version of TrySetProperty - one with Thread.Sleep and one with Task.Delay? — RNDThoughts, Feb 13 '17 at 13:06
You also could try to `await Task.Run(() => TrySetProperty(…)` in case of the async version, as you don't really do anything async in your property setter — VMAtm, Feb 13 '17 at 13:44

score 2 · Accepted Answer · answered Feb 13 '17 at 14:31

I would recommend:

Making a fully-asynchronous version if possible. REST APIs are naturally asynchronous, but some client libraries are still outdated (i.e., have only synchronous methods).
Expose only an asynchronous version if possible (since the operation is naturally asynchronous). If you must support synchronous APIs (e.g., for backwards compatibility), then use the boolean argument hack in my article on brownfield async.
Use Polly for retry logic.

Assuming that you've got a fully-asynchronous version of GraphClient.SetExtendedProperty working, then your code could look like this:

private static readonly Policy syncPolicy = Policy.Handle<Exception>().WaitAndRetry(5, _ => TimeSpan.FromSeconds(1));
private static readonly Policy asyncPolicy = Policy.Handle<Exception>().WaitAndRetryAsync(5, _ => TimeSpan.FromSeconds(1));

private static async Task TrySetProperty(GraphObject user, string propertyName, string value, bool sync)
{
    if (sync)
        syncPolicy.Execute(() => GraphClient.SetExtendedProperty(user, propertyName, value));
    else
        await asyncPolicy.ExecuteAsync(() => GraphClient.SetExtendedPropertyAsync(user, propertyName, value));
}

public static Task TrySetPropertyAsync(GraphObject user, string propertyName, string value) =>
    TrySetProperty(user, propertyName, value, sync: false);
public static void TrySetProperty(GraphObject user, string propertyName, string value) =>
    TrySetProperty(user, propertyName, value, sync: true).GetAwaiter().GetResult();

If your TrySetProperty logic really is that simple (i.e., it literally only calls a single method on GraphClient), then you can do away with the boolean argument hack for simpler code:

private static readonly Policy syncPolicy = Policy.Handle<Exception>().WaitAndRetry(5, _ => TimeSpan.FromSeconds(1));
private static readonly Policy asyncPolicy = Policy.Handle<Exception>().WaitAndRetryAsync(5, _ => TimeSpan.FromSeconds(1));

public static async Task TrySetPropertyAsync(GraphObject user, string propertyName, string value)
{
  await asyncPolicy.ExecuteAsync(() => GraphClient.SetExtendedPropertyAsync(user, propertyName, value));
}

public static void TrySetProperty(GraphObject user, string propertyName, string value)
{
  syncPolicy.Execute(() => GraphClient.SetExtendedProperty(user, propertyName, value));
}

Thread.Sleep or Task.Delay in a synchronous method with both a sync and async caller

1 Answers1