2

For speed we sometimes return response to consumer before state is saved in DB. Sometimes (Mostly for our automated consumers) this can break because the want to make actions on the saved data before it is saved. I wrote this little helper

public async Task<TEntity> GetWithRetry<TEntity>(Expression<Func<TEntity, bool>> predicate, string errorOnTimeout) where TEntity : class
{
    var stopwatch = new Stopwatch();
    stopwatch.Start();

    do
    {
        var entity = await _context.DbSet<TEntity>().FirstOrDefaultAsync(predicate);
        if(entity == null)
            await Task.Delay(100);
        else
            return entity;
    } while(stopwatch.Elapsed < TimeSpan.FromSeconds(1000));

    throw new Exception(errorOnTimeout);
}

Used like

var existingBooking = await GetWithRetry<Booking>(b => b.BookingKey == confirmCommand.BookingKey, "Booking key not found");

Any pitfalls? Task.Delay should scale well since it returns the thread to the pool, and if the data exists in DB the first time around it should not give much more overhead than an extra wrapped task?

This is the current version of the code:

public async Task<TEntity> FirstAsync<TEntity>(Expression<Func<TEntity, bool>> predicate, string errorOnTimeout, TimeSpan? timeout = null) where TEntity : class
{
    var entity = await FirstAsyncOrDefault(predicate, timeout);
    if(entity != null) return entity;

    throw new Exception(errorOnTimeout);
}

public async Task<TEntity> FirstOrDefaultAsync<TEntity>(Expression<Func<TEntity, bool>> predicate, TimeSpan? timeout = null) where TEntity : class
{
    var stopwatch = new Stopwatch();
    stopwatch.Start();

    if(timeout == null)
        timeout = TimeSpan.FromSeconds(1);

    do
    {
        var entity = await DbSet<TEntity>().FirstOrDefaultAsync(predicate);
        if(entity != null) return entity;

        await Task.Delay(100);

    } while(stopwatch.Elapsed < timeout);

    return null;
}
  • This will probably be well-received on codereview.stackexchange.com – JᴀʏMᴇᴇ Sep 29 '15 at 13:41
  • Did as you asked, only got feedback about code standards, I dont care about that. I want to know about the Task part of the code – Anders Sep 30 '15 at 13:26
  • I don't understand this code. What is that await on Task.Delay(100) for? If you're looping while entity == null, haven't you effectively turned it into a blocking call? And you're setting up multiple continuations... I don't see how this code works at all. – Robert Harvey Sep 30 '15 at 15:12
  • @RobertHarvey, Task.Delay is like Thread.Sleep except it is non-blocking. See here. It is convenient to use when the containing method is async and therefore await can be used. – Kasey Speakman Sep 30 '15 at 21:26
  • Do you really want to wait for a thousand seconds before failing? – Erik Sep 30 '15 at 22:31
1

If you just want retries with timeout, you can use a for loop instead of Stopwatch.

const int MaxWaitTime = 1000; //ms
const int SleepTime = 100; //ms

public async Task<TEntity> GetWithRetry<TEntity>(Expression<Func<TEntity, bool>> predicate)
    where TEntity : class
{
    TEntity entity = null;
    // run while under wait time and entity is null
    for (var i = 0; i < MaxWaitTime && entity == null; i += SleepTime) 
    {
        // only delay after first attempt
        if (i != 0) await Task.Delay(SleepTime);

        entity = await _context.DbSet<TEntity>().FirstOrDefaultAsync(predicate);
    }
    // I'd let the caller decide whether to throw on null here
    return entity;
}

But

Seems like a better solution would be to add a call for your customers that only completes when the data is written. Understanding that blocking for IO is a performance problem, don't! Try returning a Task, perhaps with TaskCompletionSource<T>. Then you can run the operation asynchronously without blocking and have the server wake back up to complete the call when the Task completes.

Example batching scenario:

// api method
public async Task DoSomethingAsync(...)
{
    // using bool as dummy type, not really returning result
    var notifySource = new TaskCompletionSource<bool>();

    // pass into your data infrastructure
    // returns without performing save
    MyDatabase.AddToWriteBatch(..., notifySource);

    // wait for the task to complete
    await notifySource.Task;
}

When your data infrastructure gets around to saving the data, it notifies the caller(s) of completion:

public void Process(Batch batch) {
    ...
    // call to database
    Database.BatchWrite(batch.DataCollection);
    foreach (var taskSource in batch.TaskSources)
        taskSource.SetResult(true); // this will trigger Task completion
}

This way, you still are not blocking for IO on the API call. But you can offer the customer a way to be sure their write is committed before performing other operations.

Or

You could use the TaskCompletionSource<TEntity> for reads, and place the reads in the same queue with writes, so that the reads are guaranteed to occur after writes.

  • I want the writing to complete fast and non blocking for the consumer,im not talking about blocking for our thread pool. That is why we queue the I/O on a worker thread and return the response directly to the consumer. – Anders Oct 1 '15 at 6:43
  • That's fine, but if your customer is getting errors, they they really want to be notified when the operation completes. Why not provide that option as well? – Kasey Speakman Oct 1 '15 at 13:21
  • It cant go wrong, if it should we queue it for retry – Anders Oct 1 '15 at 13:54
  • In any case, my version of the retry at the top is less code and marginally more efficient since Stopwatch has extra timing code or system clock access under the covers. However, I still maintain when I have found myself needing this, I need to come at the problem from a different angle which would avoid it. – Kasey Speakman Oct 1 '15 at 15:05
  • Updated to include another alternative that may work better for you. – Kasey Speakman Oct 1 '15 at 15:10
0

In my experience retries like this are a bad idea.

Reasoning:

  • They make you code inherently non deterministic. How many times will you hit the db? if you got null back do you just need to wait longer or is there a problem?

  • They introduce a performance problem. If you have some process which calls this function it potentially takes timeout milliseconds to complete.

  • Threading issues. if you are performing multiple operations, say web requests, each will spawn on of these loops. if the timeout takes longer than the frequency of requests everything will grind to a halt.

If you want to do something once and event has occurred, you should have an event which triggers the process.

you can do this by having a queue of tasks and a worker process, or if its all in the same code, just .ContinueWith(NextStep)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.