SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
775 stars 226 forks source link

Fix S1751: Rule should not raise on "retry on exception" pattern #590

Closed jcurl closed 7 years ago

jcurl commented 7 years ago

Description

S1751: Remove this 'return' statement or make it unconditional doesn't take into account exceptions

Repro steps

The function generates this warning

            protected virtual bool MoveWithRetry(string sourceFileName, string destFileName)
            {
                ResetRetryTimeout();
                while (true) {
                    try {
                        HBAS.IO.File.MoveReplace(sourceFileName, destFileName);
                        return true;
                    } catch (System.Exception e) {
                        int ecode = System.Runtime.InteropServices.Marshal.GetHRForException(e);
                        if (ecode == unchecked((int)0x80070005) ||
                            ecode == unchecked((int)0x80070020)) {
                            // The reason was ERROR_ACCESS_DENIED or ERROR_SHARING_VIOLATION
                            if (!WaitRetryTimeout()) throw;
                        } else if (ecode == unchecked((int)0x80070002)) {
                            // The reason was ERROR_FILE_NOT_FOUND
                            return false;
                        } else {
                            throw;
                        }
                    }
                };
            }

If the method HBAS.IO.File.MoveReplace raises an exception, it will go in the catch. There is a section there where it will exit the catch and go back to the beginning of the while loop, particularly if it's one of two types (0x80070005 or 0x80070020) and there's no timeout which checks against a Stopwatch object.

Expected behavior

This false positive shouldn't occur

Actual behavior

See expected behaviour

Known workarounds

None

Related information

michalb-sonar commented 7 years ago

Thanks for another good report @jcurl. It is a fairly common pattern to retry on exception. Indeed, the rule should not raise here.