Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jcurl opened this issue Jul 21, 2017 · 1 comment
Closed

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

jcurl opened this issue Jul 21, 2017 · 1 comment
Assignees
Labels
Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@jcurl
Copy link

jcurl commented Jul 21, 2017

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

  • SonarC# Version 3.3.0
  • Visual Studio Version 2015
@michalb-sonar
Copy link
Contributor

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

@michalb-sonar michalb-sonar added the Type: False Positive Rule IS triggered when it shouldn't be. label Jul 24, 2017
@michalb-sonar michalb-sonar added this to the near-future milestone Jul 24, 2017
@michalb-sonar michalb-sonar changed the title S1751 false alarm when used with exceptions Fix S1751: Rule should not raise on "retry on exception" pattern. Jul 24, 2017
@Evangelink Evangelink modified the milestones: 6.4, near-future Jul 25, 2017
@Evangelink Evangelink self-assigned this Jul 25, 2017
@Evangelink Evangelink modified the milestones: 6.3, 6.4 Jul 25, 2017
@Evangelink Evangelink changed the title Fix S1751: Rule should not raise on "retry on exception" pattern. Fix S1751: Rule should not raise on "retry on exception" pattern Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

No branches or pull requests

3 participants