S3546: False-positives happen when using nested try-cache-finally

Clean Code Report False-positive / False-negative...
,
1

Make sure to read this post before raising a thread here:

Then tell us:

  • What language is this for?
    java
  • Which rule?
    java:S3546: Custom resources should be closed
  • Why do you believe it’s a false-positive/false-negative?
    false-positive
    False-positives happen when using nested try-cache-finally.
  • Are you using
    • SonarQube - which version?
      Version 8.9.2 (build 46101)
  • How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)

I use this project: GitHub - dianping/cat: CAT 作为服务端项目基础组件，提供了 Java, C/C++, Node.js, Python, Go 等多语言客户端，已经在美团点评的基础架构中间件框架（MVC框架，RPC框架，数据库框架，缓存框架等，消息队列，配置系统等）深度集成，为美团点评各业务线提供系统丰富的性能指标、健康状况、实时告警等。

package org.sonar.custom;

import com.dianping.cat.Cat;
import com.dianping.cat.message.Transaction;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.*;

// Parameters for this test should be:
//    factoryMethod = com.dianping.cat.Cat#newTransaction(java.lang.String,java.lang.String)
//    closingMethod = com.dianping.cat.message.Transaction#complete()


abstract class CatTransaction {

  private static Logger logger = LoggerFactory.getLogger(CatTransaction.class);

  void correct() {
    Transaction tx = Cat.newTransaction("1", "1");
    try {
      tx.setStatus(Transaction.SUCCESS);
    } catch (Exception e) {
      logger.error("failed", e);
      tx.setStatus(e);
      throw e;
    } finally {
      tx.complete();
    }
  }

  void wrong() {
    try {
      Transaction tx = Cat.newTransaction("1", "1");
      try {
        tx.setStatus(Transaction.SUCCESS);
      } catch (Exception e) {
        logger.error("failed", e);
        tx.setStatus(e);
        throw e;
      } finally {
        tx.complete();
      }
    } catch (Exception e) {
      logger.error("error", e);
    }
  }

}

test return java.lang.AssertionError: Unexpected at [34], but I don’t know why the first method is correct and the second method is wrong.

2

Hi,

Your version is past EOL. You should upgrade to either the latest version or the current LTA (long-term active version) at your earliest convenience. Your upgrade path is:

8.9.2 → 9.9.6 → 10.6 (last step optional)

You may find these resources helpful:

If you have questions about upgrading, feel free to open a new thread for that here.

If your issue persists after upgrade, please come back to us.

3

Thanks for the reply, I also tested on the new version(Version 9.9.4 (build 87374)) of sonar and the same problem exists. Please help to solve this problem

6

Thank you, @644262163, for reporting this issue.

The rule S3546 uses the Symbolic Execution engine, which is not actively maintained and will probably be archived. Said this, I believe that this is not a false positive. Let’s analyze the code example:

void wrong() {
    try {
      Transaction tx = Cat.newTransaction("1", "1");
      try {
        tx.setStatus(Transaction.SUCCESS);
      } catch (Exception e) {
        logger.error("failed", e);
        tx.setStatus(e);
        throw e;
      } finally {
        tx.complete();
      }
    } catch (Exception e) {
      logger.error("error", e);
    }
  }

The resource Transaction tx is open in the external try block and expected to be closed in its finally block. It is wrong to close the resource in a nested finally block that may not be reached. A compliant solution would be:

void wrong() {
    try {
      Transaction tx = Cat.newTransaction("1", "1");
      try {
        tx.setStatus(Transaction.SUCCESS);
      } catch (Exception e) {
        logger.error("failed", e);
        tx.setStatus(e);
        throw e;
      } 
    } catch (Exception e) {
      logger.error("error", e);
    } finally {
      tx.complete();
    }
  }

Cheers,
Angelo

1 Like