Как я могу улучшить этот сценарий повторной попытки исключения? - PullRequest
22 голосов
/ 16 февраля 2010

У меня есть метод веб-службы, который я вызываю, который является сторонним и находится за пределами моего домена. По какой-то причине время от времени веб-служба дает сбой со временем ожидания шлюза. Его прерывистость и вызов к нему сразу после неудачной попытки могут быть успешными.

Теперь у меня осталась дилемма кодирования, у меня есть код, который должен справиться с задачей, но код выглядит как любительский час, как вы увидите ниже.

Это действительно плохой код или приемлемо с учетом использования? Если это не приемлемо, как я могу улучшить это?

Пожалуйста, старайтесь не смотреть прямо на лицо.

try
{
    MDO = OperationsWebService.MessageDownload(MI);
}
catch
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
    }
    catch
    {
        try
        {
            MDO = OperationsWebService.MessageDownload(MI);
        }
        catch
        {
            try
            {
                MDO = OperationsWebService.MessageDownload(MI);
            }
            catch 
            {
                try
                {
                    MDO = OperationsWebService.MessageDownload(MI);
                }
                catch (Exception ex)
                {
                    // 5 retries, ok now log and deal with the error.
                }
            }
        }
    }
}

Ответы [ 10 ]

21 голосов
/ 16 февраля 2010

Вы можете сделать это в цикле.

Exception firstEx = null;
for(int i=0; i<5; i++) 
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
        firstEx = null;
        break; 
    }
    catch(Exception ex)
    {
        if (firstEx == null) 
        {
            firstEx = ex;
        }
        Thread.Sleep(100 * (i + 1));
    }
}
if (firstEx != null) 
{
    throw new Exception("WebService call failed after 5 retries.", firstEx);
}
12 голосов
/ 16 февраля 2010

Вот еще один способ, которым вы можете попробовать:

// Easier to change if you decide that 5 retries isn't right for you
Exception exceptionKeeper = null;
for (int i = 0; i < MAX_RETRIES; ++i)
{
    try
    {
       MDO = OperationsWebService.MessageDownload(MI);
       break;  // correct point from Joe - thanks.
    }
    catch (Exception ex)
    {
        exceptionKeeper = ex;
        // 5 retries, ok now log and deal with the error.
    }  
}

Я думаю, что это лучше документирует намерения. Это также меньше кода; проще в обслуживании.

11 голосов
/ 16 февраля 2010

Все ответы до сих пор предполагают, что реакцией на любое исключение должно быть повторение операции. Это хорошее предположение, вплоть до ложного. Вы можете легко повторить операцию, которая повреждает вашу систему, все потому, что вы не проверяли тип исключения.

Вы должны почти никогда использовать только «catch» или «catch (Exception ex)». Поймайте более конкретное исключение, которое, как вы знаете, вы можете безопасно восстановить.

2 голосов
/ 16 февраля 2010

Попробуйте цикл с некоторым ограничением:

int retryCount = 5;
var done = false;
Exception error = null;
while (!done && retryCount > 0)
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
        done = true;
    }
    catch (Exception ex)
    {
        error = ex;
    }
    if (done)
        break;

    retryCount--;
}
1 голос
/ 18 февраля 2011

Я использую следующий универсальный метод для сценария повтора. Я особенно хочу обратить внимание на метод PreserveStackTrace, который помогает сохранить полную трассировку стека вызовов, поскольку (как я понял сложный путь) ни throw, ни throw ex не дают полного стека вызовов информация трассировки.

public static void RetryBeforeThrow<T>(Action action, int retries, int timeout) where T : Exception
{
    int tries = 1;

    do
    {
        try
        {
            action();
            return;
        }
        catch (T ex)
        {
            if (retries <= 0)
            {
                PreserveStackTrace(ex);
                throw;
            }

            Thread.Sleep(timeout);
        }
    }
    while (tries++ < retries);
}

/// <summary>
/// Sets a flag on an <see cref="T:System.Exception"/> so that all the stack trace information is preserved 
/// when the exception is re-thrown.
/// </summary>
/// <remarks>This is useful because "throw" removes information, such as the original stack frame.</remarks>
/// <see href="http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspx"/>
public static void PreserveStackTrace(Exception ex)
{
    MethodInfo preserveStackTrace = typeof(Exception).GetMethod("InternalPreserveStackTrace", BindingFlags.Instance | BindingFlags.NonPublic);
    preserveStackTrace.Invoke(ex, null);
}
1 голос
/ 18 декабря 2010

Вот некоторая логика повторения, которую мы используем.Мы не делаем этого много, и я собирался вытащить это и задокументировать как наш Retry Pattern / Standard.Когда я впервые написал это, мне пришлось это сделать, поэтому я пришел сюда, чтобы проверить, правильно ли я это делаю.Похоже, я был.Версия ниже полностью прокомментирована.Ниже приведена информация о некомментированной версии.

#region Retry logic for SomeWebService.MyMethod
// The following code wraps SomeWebService.MyMethod in retry logic
// in an attempt to account for network failures, timeouts, etc.

// Declare the return object for SomeWebService.MyMethod outside of
// the following for{} and try{} code so that we have it afterwards.
MyMethodResult result = null;

// This logic will attempt to retry the call to SomeWebService.MyMethod
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);

        // If we didn't get an exception, then that (most likely) means that the
        // call was successful so we can break out of the retry logic.
        break;
    }
    catch (Exception ex)
    {
        // Ideally we want to only catch and act on specific
        // exceptions related to the failure. However, in our
        // testing, we found that the exception could be any type
        // (service unavailable, timeout, database failure, etc.)
        // and attempting to trap every exception that was retryable
        // was burdensome. It was easier to just retry everything
        // regardless of the cause of the exception. YMMV. Do what is
        // appropriate for your scenario.

        // Need to check to see if there will be another retry attempt allowed.
        if (retryAttempt < Config.MaxRetryAttempts)
        {
            // Log that we are re-trying
            Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);

            // Put the thread to sleep. Rather than using a straight time value for each
            // iteration, we are going to multiply the sleep time by how many times we
            // have currently tried to call the method. This will allow for an easy way to
            // cover a broader range of time without having to use higher retry counts or timeouts.
            // For example, if MaxRetryAttempts = 10 and RetrySleepSeconds = 60, the coverage will
            // be as follows:
            // - Retry #1 - Sleep for 1 minute
            // - Retry #2 - Sleep for 2 minutes (covering three minutes total)
            // - Retry #10 - Sleep for 10 minutes (and will have covered almost an hour of downtime)
            Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
        }
        else
        {
            // If we made it here, we have tried to call the method several
            // times without any luck. Time to give up and move on.

            // Moving on could either mean:
            // A) Logging the exception and moving on to the next item.
            Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
            // B) Throwing the exception for the program to deal with.
            throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
            // Or both. Your code, your call.
        }
    }
}
#endregion

Мне нравится пример Сэмюэля Неффа об использовании переменной исключения, чтобы увидеть, полностью ли она потерпела неудачу или нет.Это сделало бы некоторые оценки в моей логике немного проще.Я мог бы пойти в любую сторону.Не уверен, что один из способов имеет значительное преимущество перед другим.Однако на данный момент я не собираюсь менять то, как мы это делаем.Важно документировать, что вы делаете и почему, чтобы какой-то идиот не оказался позади вас и не испортил все.

Только для того, чтобы пнуть, чтобы получить лучшее представление, если код корочеили чище, так или иначе, я вытащил все комментарии.У них вышло ровно столько же строк.Я скомпилировал две версии и провел их по метрикам кода рефлектора и получил следующее:

Metric: Inside-Catch / Outside-For
CodeSize: 197 / 185
CyclomaticComplexity: 3 / 3
Instructions: 79 / 80
Locals: 6 / 7

Финальная логика исключений внутри улова (22 строки):

MyMethodResult result = null;
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);
        break;
    }
    catch (Exception ex)
    {
        if (retryAttempt < Config.MaxRetryAttempts)
        {
            Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);
            Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
        }
        else
        {
            Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
            throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
        }
    }
}

Finalлогика исключений после цикла for (22 строки):

MyMethodResult result = null;
Exception retryException = null;
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);
        retryException = null;
        break;
    }
    catch (Exception ex)
    {
        retryException = ex;
        Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);
        Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
    }
}
if (retryException != null)
{
    Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
    throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
}
1 голос
/ 16 февраля 2010

Вы должны использовать рекурсию (или цикл) и повторять попытки только в том случае, если вы получили ожидаемую ошибку.

Например:

static void TryExecute<TException>(Action method, Func<TException, bool> retryFilter, int maxRetries) where TException : Exception {
    try {
        method();
    } catch(TException ex) {
        if (maxRetries > 0 && retryFilter(ex))
            TryExecute(method, retryFilter, maxRetries - 1);
        else
            throw;
    }
}

РЕДАКТИРОВАТЬ : с петлей:

static void TryExecute<TException>(Action method, Func<TException, bool> retryFilter, int maxRetries) where TException : Exception {
    while (true) {
        try {
            method();
            return;
        } catch(TException ex) {
            if (maxRetries > 0 && retryFilter(ex))
                maxRetries--;
            else
                throw;
        }
    }
}

Вы можете попытаться предотвратить будущие ошибки в retryFilter, возможно Thread.Sleep.

Если последняя повторная попытка не удалась, это вызовет последнее исключение.

0 голосов
/ 16 февраля 2010

Кажется, у вас есть ответы, которые вам нужны, но я решил опубликовать эту ссылку, Что такое политика действий? , которую я нашел, чтобы предоставить гораздо более элегантное решение. У Lokad есть несколько довольно лабиринтных реализаций, но логика этого парня довольно солидна, и конечный код, который вы в итоге напишите, будет красивым и простым.

0 голосов
/ 16 февраля 2010

Как и все остальные отмечают, правильный подход - заключить ваш try / catch в некоторый цикл с помощью некоторого MAX_RETRY.

Вы можете также рассмотреть возможность добавления тайм-аута между каждой итерацией цикла. В противном случае вы, скорее всего, прогораете через свой счетчик повторов до того, как временная проблема сможет решить сама.

0 голосов
/ 16 февраля 2010
int cnt=0;
bool cont = true;
while (cont)
{
     try
     {
         MDO = OperationsWebService.MessageDownload(MI);
         cont = false; 
     }
     catch (Exception ex) 
     { 
         ++cnt;
         if (cnt == 5)
         {
             // 5 retries, ok now log and deal with the error. 
            cont = false;
         }
     } 
}

ОБНОВЛЕНО: исправлен код, основанный на комментариях.

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...