ServiceBus: SpotBugs P2 part 3 of 3 by mssfang · Pull Request #3528 · Azure/azure-sdk-for-java

mssfang · 2019-05-06T00:13:16Z

This PR contains:

(1) P2 SpotBugs: search [SpotBugs-P2] for detail explanation
(2) CheckStyles errors to these changed files

...vicebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java

mssfang · 2019-05-06T00:20:36Z

...c/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescriptionSerializer.java

                                            sd.forwardDeadLetteredMessagesTo = fwdDlq.getNodeValue();
                                        }
                                        break;
+                                    default:


[SpotBugs-P2]: Switch statement found in com.microsoft.azure.servicebus.management.SubscriptionDescriptionSerializer.parseFromEntry(String, Node) where default case is missing

mssfang · 2019-05-06T00:21:16Z

...c/main/java/com/microsoft/azure/servicebus/management/SubscriptionRuntimeInfoSerializer.java

                                                    case "TransferDeadLetterMessageCount":
                                                        runtimeInfo.getMessageCountDetails().setTransferDeadLetterMessageCount(Long.parseLong(element.getFirstChild().getNodeValue()));
                                                        break;
+                                                    default:


[SpotBugs-P2]: Switch statement found in com.microsoft.azure.servicebus.management.SubscriptionRuntimeInfoSerializer.parseFromEntry(String, Node) where default case is missing

...ure-servicebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescription.java

mssfang · 2019-05-06T00:22:55Z

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescriptionSerializer.java

                                    case "SupportOrdering":
                                        td.supportOrdering = Boolean.parseBoolean(element.getFirstChild().getNodeValue());
                                        break;
+                                    default:


[SpotBugs-P2] Switch statement found in com.microsoft.azure.servicebus.management.TopicDescriptionSerializer.parseFromEntry(Node) where default case is missing

mssfang · 2019-05-06T00:23:20Z

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicRuntimeInfoSerializer.java

                                                    case "TransferDeadLetterMessageCount":
                                                        topicRuntimeInfo.getMessageCountDetails().setTransferDeadLetterMessageCount(Long.parseLong(element.getFirstChild().getNodeValue()));
                                                        break;
+                                                    default:


[SpotBugs-P2]: Switch statement found in com.microsoft.azure.servicebus.management.TopicRuntimeInfoSerializer.parseFromEntry(Node) where default case is missing

mssfang · 2019-05-06T00:27:19Z

...zure-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/ClientConstants.java

        final Properties properties = new Properties();
+        InputStream clientPropInputStream = null;
        try {
-            properties.load(ClientConstants.class.getResourceAsStream("/client.properties"));


[SpotBugs-P2]: com.microsoft.azure.servicebus.primitives.ClientConstants.getClientVersion() may fail to clean up java.io.InputStream

This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operation.
In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.
This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. For sending feedback, check:
contributing guideline
malinglist
In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.
See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes, for a description of the analysis technique.

mssfang · 2019-05-06T00:28:29Z

...ane/azure-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/Controller.java

    }

    synchronized CompletableFuture<Void> initializeAsync() {
-        if (this.isInitialized) {


[SpotBugs-P2]: Inconsistent synchronization of com.microsoft.azure.servicebus.primitives.Controller.isInitialized; locked 50% of time

The fields of this class appear to be accessed inconsistently with respect to synchronization. This bug report indicates that the bug pattern detector judged that
The class contains a mix of locked and unlocked accesses,
The class is not annotated as javax.annotation.concurrent.NotThreadSafe,
At least one locked access was performed by one of the class's own methods, and
The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads
A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.
You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.
Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held. Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

azuresdkci · 2019-05-06T00:31:32Z

Can one of the admins verify this patch?

...vicebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java

conniey

Some changes with those null pointer fixes.

yvgopal · 2019-05-15T19:58:09Z

...vicebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java

-                (defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0 ||
-                        defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0))
-        {
+        if (defaultMessageTimeToLive != null


f (defaultMessageTimeToLive != null [](start = 9, length = 35)

May be you can change it to defaultMessageTimeToLive == null. That way it will throw a validation exception for the null case too. Can you do for every Description class where this null check is not done?

Do you want to have all null checking for all setter methods that pass Object as parameters in the Description class if they don't handle NPE? Such as
public void setAuthorizationRules(List authorizationRules) {
this.authorizationRules = authorizationRules;
}

...c/main/java/com/microsoft/azure/servicebus/management/SubscriptionRuntimeInfoSerializer.java

yvgopal · 2019-05-15T19:58:53Z

...ure-servicebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescription.java

    public void setDefaultMessageTimeToLive(Duration defaultMessageTimeToLive) {
-        if (defaultMessageTimeToLive != null &&
-                (defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0 ||
-                        defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0))


Same null check as I suggested in my last comment.

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescriptionSerializer.java

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicRuntimeInfoSerializer.java

yvgopal · 2019-05-15T20:01:19Z

...lane/azure-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/AsyncUtil.java

-    public static <T> void completeFuture(CompletableFuture<T> future, T result)
-    {
+    public static <T> void completeFuture(CompletableFuture<T> future, T result) {
        MessagingFactory.INTERNAL_THREAD_POOL.submit(new CompleteCallable<T>(future, result));


[](start = 73, length = 3)

Some places you removed the template type variable. Some places you didn't.

yvgopal · 2019-05-15T20:07:10Z

...-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/CoreMessageReceiver.java

-                {
+        this.timedOutUpdateStateRequestsDaemon = () -> {
+            try {
+                if (TRACE_LOGGER.isTraceEnabled()) {


if (TRACE_LOGGER.isTraceEnabled()) { [](start = 16, length = 36)

Why are you checking isTraceEnabled()? With SLF4J, you don't have to do this as long as you are using string formatting properly. Remove these isLevelEnabled calls.

yvgopal

🕐

…into ServiceBus-P2-Part3

mssfang · 2019-05-16T21:20:37Z

...zure-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/ClientConstants.java

+        } finally {
+            if (clientPropInputStream != null) {
+                try {
+                    clientPropInputStream.close();


close the clientPropInputStream

…into ServiceBus-P2-Part3

mssfang · 2019-05-17T17:11:37Z

@yvgopal There has no more spotbugs :). Can you review the changes? Thank you

yvgopal

Split SpotBugs P2 part 3

5f2d140

mssfang added Service Bus Client This issue points to a problem in the data-plane of the library. labels May 6, 2019

mssfang self-assigned this May 6, 2019

mssfang commented May 6, 2019

View reviewed changes

...vicebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java Outdated Show resolved Hide resolved

mssfang commented May 6, 2019

View reviewed changes

...ure-servicebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescription.java Outdated Show resolved Hide resolved

mssfang commented May 6, 2019

View reviewed changes

...ure-servicebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescription.java Outdated Show resolved Hide resolved

mssfang commented May 6, 2019

View reviewed changes

mssfang requested review from conniey and yvgopal May 6, 2019 01:05

mssfang changed the title ~~ServiceBugs: SpotBugs P2 part 3 of 3~~ ServiceBus: SpotBugs P2 part 3 of 3 May 7, 2019

conniey reviewed May 8, 2019

View reviewed changes

...vicebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java Outdated Show resolved Hide resolved

conniey requested changes May 8, 2019

View reviewed changes

yvgopal reviewed May 15, 2019

View reviewed changes

...c/main/java/com/microsoft/azure/servicebus/management/SubscriptionRuntimeInfoSerializer.java Outdated Show resolved Hide resolved

yvgopal reviewed May 15, 2019

View reviewed changes

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescriptionSerializer.java Outdated Show resolved Hide resolved

yvgopal reviewed May 15, 2019

View reviewed changes

...ebus/src/main/java/com/microsoft/azure/servicebus/management/TopicRuntimeInfoSerializer.java Outdated Show resolved Hide resolved

yvgopal reviewed May 15, 2019

View reviewed changes

yvgopal requested changes May 15, 2019

View reviewed changes

shafang added 3 commits May 15, 2019 18:19

fixes based on new feedback

082b235

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

34d49a7

…into ServiceBus-P2-Part3

resolved conflict

53bea12

mssfang requested review from conniey and yvgopal May 16, 2019 18:36

resolve conflict

7fefa13

mssfang commented May 16, 2019

View reviewed changes

shafang added 4 commits May 16, 2019 14:33

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

c047f45

…into ServiceBus-P2-Part3

resolve conflicts

5753647

add rest of suppression of spotbugs

5f0e741

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

e58a4bc

…into ServiceBus-P2-Part3

yvgopal approved these changes May 17, 2019

View reviewed changes

Merge branch 'master' into ServiceBus-P2-Part3

c437495

mssfang merged commit 435a7b0 into Azure:master May 17, 2019

mssfang deleted the ServiceBus-P2-Part3 branch May 17, 2019 20:53

Conversation

mssfang commented May 6, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azuresdkci commented May 6, 2019

Uh oh!

Uh oh!

conniey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssfang May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yvgopal left a comment

Choose a reason for hiding this comment

Uh oh!

mssfang May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssfang commented May 17, 2019

Uh oh!

yvgopal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

mssfang May 16, 2019 •

edited

Loading

mssfang May 16, 2019 •

edited

Loading