Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-12187

Review in source the paper "Simple Testing Can Prevent Most Critical Failures"

    Details

    • Type: Bug
    • Status: Open
    • Priority: Critical
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Review the helpful paper https://www.usenix.org/system/files/conference/osdi14/osdi14-paper-yuan.pdf

      It describes 'catastrophic failures', especially issues where exceptions are thrown but not properly handled. Their static analysis tool Aspirator turns up a bunch of the obvious offenders (Lets add to test-patch.sh alongside findbugs?). This issue is about going through code base making sub-issues to root out these and others (Don't we have the test described in figure #6 already? I thought we did? If we don't, need to add).

      1. abortInOvercatch.warnings.txt
        5 kB
        Ding Yuan
      2. emptyCatch.warnings.txt
        60 kB
        Ding Yuan
      3. HBASE-12187.patch
        29 kB
        Ding Yuan
      4. todoInCatch.warnings.txt
        2 kB
        Ding Yuan

        Issue Links

          Activity

          Hide
          d.yuan Ding Yuan added a comment -

          Hi Stack,
          I'm the author of this paper you mentioned. Thanks for your attentions.
          I understand that it may be more convenient to build the checks implemented by aspirator into the tools you guys already use. I am very happy to help here. Do you think it would be helpful to build these checks into FindBugs or other tools you guys use? If so, I'm happy to give it a shot.

          Show
          d.yuan Ding Yuan added a comment - Hi Stack, I'm the author of this paper you mentioned. Thanks for your attentions. I understand that it may be more convenient to build the checks implemented by aspirator into the tools you guys already use. I am very happy to help here. Do you think it would be helpful to build these checks into FindBugs or other tools you guys use? If so, I'm happy to give it a shot.
          Hide
          stack stack added a comment -

          Ding Yuan Thank you for paper, your findings, your previous patches, and your now showing up to offer help. Much appreciated. We have a bunch of work to do to address items raised in your paper.

          Currently we run findbugs and other checks out of a script as part of precommit test on patches. The script we run is at ./dev-test/test-patch.sh. We could add stuff in here but looking at aspirator as-is, it looks a bit tough to integrate (build Chord then put in place files...). How hard to integrate into findbugs? Then your work would get wide dissemination?

          Show
          stack stack added a comment - Ding Yuan Thank you for paper, your findings, your previous patches, and your now showing up to offer help. Much appreciated. We have a bunch of work to do to address items raised in your paper. Currently we run findbugs and other checks out of a script as part of precommit test on patches. The script we run is at ./dev-test/test-patch.sh. We could add stuff in here but looking at aspirator as-is, it looks a bit tough to integrate (build Chord then put in place files...). How hard to integrate into findbugs? Then your work would get wide dissemination?
          Hide
          apurtell Andrew Purtell added a comment -

          We are also looking at introducing static code checks at compile time via HBASE-11912, which could be another implementation option.

          Show
          apurtell Andrew Purtell added a comment - We are also looking at introducing static code checks at compile time via HBASE-11912 , which could be another implementation option.
          Hide
          d.yuan Ding Yuan added a comment -

          Hi Stack and Andrew,
          It seems from the discussion of HBASE-11912 you guys think error-prone might be a better approach, then I will try to add a few checks into error-prone to catch the trivial bug patterns implemented by aspirator. I am not familiar with error-prone's AST representation, so I am not sure whether some of the checks can be easily implemented, but it shouldn't be complicated anyway. Afterward I can also try to provide a FindBugs implementation.

          Do you guys have your own error-prone repository or I should simply work on the one from https://code.google.com/p/error-prone/?

          cheers,

          Show
          d.yuan Ding Yuan added a comment - Hi Stack and Andrew, It seems from the discussion of HBASE-11912 you guys think error-prone might be a better approach, then I will try to add a few checks into error-prone to catch the trivial bug patterns implemented by aspirator. I am not familiar with error-prone's AST representation, so I am not sure whether some of the checks can be easily implemented, but it shouldn't be complicated anyway. Afterward I can also try to provide a FindBugs implementation. Do you guys have your own error-prone repository or I should simply work on the one from https://code.google.com/p/error-prone/? cheers,
          Hide
          apurtell Andrew Purtell added a comment -

          We don't have our own repository (yet)

          Show
          apurtell Andrew Purtell added a comment - We don't have our own repository (yet)
          Hide
          stack stack added a comment -

          Ding Yuan What Andrew Purtell said but if you need us to check it in local, we have the dev-tools dir where we could add it a while if you need a guinea pig, etc.

          Show
          stack stack added a comment - Ding Yuan What Andrew Purtell said but if you need us to check it in local, we have the dev-tools dir where we could add it a while if you need a guinea pig, etc.
          Hide
          d.yuan Ding Yuan added a comment -

          Great! I will work on it and get back to you in a few days.

          Show
          d.yuan Ding Yuan added a comment - Great! I will work on it and get back to you in a few days.
          Hide
          d.yuan Ding Yuan added a comment -

          I have implemented the three checks from aspirator into error-prone version 1.1.2. These three checks are:
          (1). Catch block that ignores exception (including containing only a log printing statement);
          (2). Aborting the system on exception over-catch;
          (3). Catch block containing "TODO" or "FIXME" in comments

          Among them, (1) is a bit complicated since I included quite a few false positive suppression heuristics as described in the paper.

          I have tested all three checks on HBase-0.99.0. The first check found 111 cases, while the other two found less than 10 each. I have attached the reported cases as attachments.

          Currently I assigned all of the three checks as "ERROR" severity. So if one thinks that a case is fine, an annotation like "@SupressWarnings("EmptyCatch")" is needed to get the compilation to succeed.

          I am attaching the patch to error-prone v1.1.2, which contains the three added checks. I have also uploaded my error-prone repository to:
          https://github.com/diy1/error-prone-aspirator

          Please let me know how i can further help.
          cheers,
          ding

          Show
          d.yuan Ding Yuan added a comment - I have implemented the three checks from aspirator into error-prone version 1.1.2. These three checks are: (1). Catch block that ignores exception (including containing only a log printing statement); (2). Aborting the system on exception over-catch; (3). Catch block containing "TODO" or "FIXME" in comments Among them, (1) is a bit complicated since I included quite a few false positive suppression heuristics as described in the paper. I have tested all three checks on HBase-0.99.0. The first check found 111 cases, while the other two found less than 10 each. I have attached the reported cases as attachments. Currently I assigned all of the three checks as "ERROR" severity. So if one thinks that a case is fine, an annotation like "@SupressWarnings("EmptyCatch")" is needed to get the compilation to succeed. I am attaching the patch to error-prone v1.1.2, which contains the three added checks. I have also uploaded my error-prone repository to: https://github.com/diy1/error-prone-aspirator Please let me know how i can further help. cheers, ding
          Hide
          busbey Sean Busbey added a comment -

          Ding Yuan, did your changes to error-prone ever get accepted into the main error-prone repository? I'm having some difficulty finding them.

          Show
          busbey Sean Busbey added a comment - Ding Yuan , did your changes to error-prone ever get accepted into the main error-prone repository? I'm having some difficulty finding them.
          Hide
          d.yuan Ding Yuan added a comment -

          Hi Sean Busbey, i don't think so. I remember I sent error-prone's developers the patches last year via google group, but I can no longer find those discussion threads (seems they migrated the whole thing to github). Should I send the patch to the error-prone devs again or you guys are hosting error-prone repository locally?

          Show
          d.yuan Ding Yuan added a comment - Hi Sean Busbey , i don't think so. I remember I sent error-prone's developers the patches last year via google group, but I can no longer find those discussion threads (seems they migrated the whole thing to github). Should I send the patch to the error-prone devs again or you guys are hosting error-prone repository locally?
          Hide
          busbey Sean Busbey added a comment -

          we only run upstream releases of error-prone. It'd be great if you could submit a pull request to get the changes into the main repo. I suspect a PR on github (i.e. https://github.com/google/error-prone/pulls ) will get more traction than a post to their google group.

          Show
          busbey Sean Busbey added a comment - we only run upstream releases of error-prone. It'd be great if you could submit a pull request to get the changes into the main repo. I suspect a PR on github (i.e. https://github.com/google/error-prone/pulls ) will get more traction than a post to their google group.
          Hide
          mdrob Mike Drob added a comment -

          Filing a subtask specifically for adding the Aspirator rules.

          Show
          mdrob Mike Drob added a comment - Filing a subtask specifically for adding the Aspirator rules.

            People

            • Assignee:
              Unassigned
              Reporter:
              stack stack
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:

                Development