Skip to content

ServiceBus: SpotBugs P2 part 3 of 3 #3528

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.

Sign up for GitHub

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

Merged
mssfang merged 10 commits into from
May 17, 2019
Merged

ServiceBus: SpotBugs P2 part 3 of 3 #3528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5f2d140
Split SpotBugs P2 part 3
May 6, 2019
082b235
fixes based on new feedback
May 16, 2019
34d49a7
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
May 16, 2019
53bea12
resolved conflict
May 16, 2019
7fefa13
resolve conflict
May 16, 2019
c047f45
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
May 16, 2019
5753647
resolve conflicts
May 17, 2019
5f0e741
add rest of suppression of spotbugs
May 17, 2019
e58a4bc
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …
May 17, 2019
c437495
Merge branch 'master' into ServiceBus-P2-Part3
mssfang May 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions eng/code-quality-reports/src/main/resources/spotbugs/spotbugs-exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,18 @@

<!-- The return value is ignored for purpose -->
<Match>
<Class name="class com.microsoft.azure.servicebus.primitives.AsyncUtil"/>
<Method name="~(completeFuture|completeFutureExceptionally)"/>
<Class name="~com\.microsoft\.azure\.servicebus\.primitives\.(AsyncUtil|RequestResponseLink\$InternalReceiver)"/>
<Method name="~(completeFuture|completeFutureExceptionally|run|onReceiveComplete)"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
</Match>

<!-- Unreachable Path -->
<Match>
<Class name="com.microsoft.azure.servicebus.primitives.RequestResponseLink$InternalSender"/>
<Local name="encodedPair"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_EXCEPTION"/>
</Match>

<!-- The casting is correct, it is beyond the SpotBugs's ability to determine the correctness -->
<Match>
<Class name="com.microsoft.azure.servicebus.primitives.RequestResponseUtils"/>
Expand All @@ -389,4 +396,10 @@
<Bug pattern="PZLA_PREFER_ZERO_LENGTH_ARRAYS"/>
</Match>

<!-- Unstable implementation. It is in Service team's to-do-list -->
<Match>
<Class name="com.microsoft.azure.servicebus.primitives.CoreMessageReceiver"/>
<Method name="onReceiveComplete"/>
<Bug pattern="AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION"/>
</Match>
</FindBugsFilter>
3 changes: 3 additions & 0 deletions ...-servicebus/src/main/java/com/microsoft/azure/servicebus/management/QueueDescription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ public Duration getLockDuration() {
* @param lockDuration - The duration of a peek lock. Max value is 5 minutes.
*/
public void setLockDuration(Duration lockDuration) {
if (lockDuration == null) {
throw new IllegalArgumentException("Value cannot be null");
}
this.lockDuration = lockDuration;
if (this.lockDuration.compareTo(ManagementClientConstants.MAX_DURATION) > 0) {
this.lockDuration = ManagementClientConstants.MAX_DURATION;
Expand Down
13 changes: 8 additions & 5 deletions ...ebus/src/main/java/com/microsoft/azure/servicebus/management/SubscriptionDescription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ public Duration getLockDuration() {
* @param lockDuration - The duration of a peek lock. Max value is 5 minutes.
*/
public void setLockDuration(Duration lockDuration) {
if (lockDuration == null) {
throw new IllegalArgumentException("Value cannot be null");
}
this.lockDuration = lockDuration;
if (this.lockDuration.compareTo(ManagementClientConstants.MAX_DURATION) > 0) {
this.lockDuration = ManagementClientConstants.MAX_DURATION;
Expand Down Expand Up @@ -134,9 +137,9 @@ public Duration getDefaultMessageTimeToLive() {
* See {@link #getDefaultMessageTimeToLive()}
*/
public void setDefaultMessageTimeToLive(Duration defaultMessageTimeToLive) {
if (defaultMessageTimeToLive != null
&& (defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0
|| defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0)) {
if (defaultMessageTimeToLive == null
|| (defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0
|| defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0)) {
throw new IllegalArgumentException(
String.format("The value must be between %s and %s.",
ManagementClientConstants.MAX_ALLOWED_TTL,
Expand All @@ -159,8 +162,8 @@ public Duration getAutoDeleteOnIdle() {
* The minimum duration is 5 minutes.
*/
public void setAutoDeleteOnIdle(Duration autoDeleteOnIdle) {
if (autoDeleteOnIdle != null
&& autoDeleteOnIdle.compareTo(ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION) < 0) {
if (autoDeleteOnIdle == null
|| autoDeleteOnIdle.compareTo(ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION) < 0) {
throw new IllegalArgumentException(
String.format("The value must be greater than %s.",
ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION));
Expand Down
11 changes: 5 additions & 6 deletions ...ain/java/com/microsoft/azure/servicebus/management/SubscriptionDescriptionSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,20 +190,17 @@ private static SubscriptionDescription parseFromEntry(String topicName, Node xEn
Node node = nList.item(i);
if (node.getNodeType() == Node.ELEMENT_NODE) {
Element element = (Element)node;
switch(element.getTagName())
{
switch (element.getTagName()) {
case "title":
sd = new SubscriptionDescription(topicName, element.getFirstChild().getNodeValue());
break;
case "content":
NodeList qdNodes = element.getFirstChild().getChildNodes();
for (int j = 0; j < qdNodes.getLength(); j++)
{
for (int j = 0; j < qdNodes.getLength(); j++) {
node = qdNodes.item(j);
if (node.getNodeType() == Node.ELEMENT_NODE) {
element = (Element) node;
switch (element.getTagName())
{
switch (element.getTagName()) {
case "RequiresSession":
sd.requiresSession = Boolean.parseBoolean(element.getFirstChild().getNodeValue());
break;
Expand Down Expand Up @@ -246,6 +243,8 @@ private static SubscriptionDescription parseFromEntry(String topicName, Node xEn
sd.forwardDeadLetteredMessagesTo = fwdDlq.getNodeValue();
}
break;
default:
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
}
}
}
Expand Down
11 changes: 5 additions & 6 deletions ...ain/java/com/microsoft/azure/servicebus/management/SubscriptionRuntimeInfoSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,17 @@ private static SubscriptionRuntimeInfo parseFromEntry(String topicPath, Node xEn
Node node = nList.item(i);
if (node.getNodeType() == Node.ELEMENT_NODE) {
Element element = (Element)node;
switch(element.getTagName())
{
switch (element.getTagName()) {
case "title":
runtimeInfo = new SubscriptionRuntimeInfo(topicPath, element.getFirstChild().getNodeValue());
break;
case "content":
NodeList qdNodes = element.getFirstChild().getChildNodes();
for (int j = 0; j < qdNodes.getLength(); j++)
{
for (int j = 0; j < qdNodes.getLength(); j++) {
node = qdNodes.item(j);
if (node.getNodeType() == Node.ELEMENT_NODE) {
element = (Element) node;
switch (element.getTagName())
{
switch (element.getTagName()) {
case "AccessedAt":
runtimeInfo.setAccessedAt(Instant.parse(element.getFirstChild().getNodeValue()));
break;
Expand Down Expand Up @@ -102,6 +99,8 @@ private static SubscriptionRuntimeInfo parseFromEntry(String topicPath, Node xEn
case "TransferDeadLetterMessageCount":
runtimeInfo.getMessageCountDetails().setTransferDeadLetterMessageCount(Long.parseLong(element.getFirstChild().getNodeValue()));
break;
default:
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
}
}
}
Expand Down
31 changes: 12 additions & 19 deletions ...-servicebus/src/main/java/com/microsoft/azure/servicebus/management/TopicDescription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,14 @@ public class TopicDescription {
* Max length is 260 chars. Cannot start or end with a slash.
* Cannot have restricted characters: '@','?','#','*'
*/
public TopicDescription(String path)
{
public TopicDescription(String path) {
this.setPath(path);
}

/**
* @return the path of the topic.
*/
public String getPath()
{
public String getPath() {
return this.path;
}

Expand All @@ -49,8 +47,7 @@ public String getPath()
* Max length is 260 chars. Cannot start or end with a slash.
* Cannot have restricted characters: '@','?','#','*'
*/
private void setPath(String path)
{
private void setPath(String path) {
EntityNameHelper.checkValidTopicName(path);
this.path = path;
}
Expand All @@ -66,8 +63,7 @@ public long getMaxSizeInMB() {
/**
* @param maxSize - Sets the maximum size of the topic in megabytes, which is the size of memory allocated for the topic.
*/
public void setMaxSizeInMB(long maxSize)
{
public void setMaxSizeInMB(long maxSize) {
this.maxSizeInMB = maxSize;
}

Expand Down Expand Up @@ -106,10 +102,9 @@ public Duration getDefaultMessageTimeToLive() {
* See {@link #getDefaultMessageTimeToLive()}
*/
public void setDefaultMessageTimeToLive(Duration defaultMessageTimeToLive) {
if (defaultMessageTimeToLive != null &&
(defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0 ||
defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0))
Copy link
Member

@yvgopal yvgopal May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same null check as I suggested in my last comment.

{
if (defaultMessageTimeToLive == null
|| (defaultMessageTimeToLive.compareTo(ManagementClientConstants.MIN_ALLOWED_TTL) < 0
|| defaultMessageTimeToLive.compareTo(ManagementClientConstants.MAX_ALLOWED_TTL) > 0)) {
throw new IllegalArgumentException(
String.format("The value must be between %s and %s.",
ManagementClientConstants.MAX_ALLOWED_TTL,
Expand All @@ -132,9 +127,8 @@ public Duration getAutoDeleteOnIdle() {
* The minimum duration is 5 minutes.
*/
public void setAutoDeleteOnIdle(Duration autoDeleteOnIdle) {
if (autoDeleteOnIdle != null &&
autoDeleteOnIdle.compareTo(ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION) < 0)
{
if (autoDeleteOnIdle == null
|| autoDeleteOnIdle.compareTo(ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION) < 0) {
throw new IllegalArgumentException(
String.format("The value must be greater than %s.",
ManagementClientConstants.MIN_ALLOWED_AUTODELETE_DURATION));
Expand All @@ -159,10 +153,9 @@ public Duration getDuplicationDetectionHistoryTimeWindow() {
* Max value is 1 day and minimum is 20 seconds.
*/
public void setDuplicationDetectionHistoryTimeWindow(Duration duplicationDetectionHistoryTimeWindow) {
if (duplicationDetectionHistoryTimeWindow != null &&
(duplicationDetectionHistoryTimeWindow.compareTo(ManagementClientConstants.MIN_DUPLICATE_HISTORY_DURATION) < 0 ||
duplicationDetectionHistoryTimeWindow.compareTo(ManagementClientConstants.MAX_DUPLICATE_HISTORY_DURATION) > 0))
{
if (duplicationDetectionHistoryTimeWindow == null
|| (duplicationDetectionHistoryTimeWindow.compareTo(ManagementClientConstants.MIN_DUPLICATE_HISTORY_DURATION) < 0
|| duplicationDetectionHistoryTimeWindow.compareTo(ManagementClientConstants.MAX_DUPLICATE_HISTORY_DURATION) > 0)) {
throw new IllegalArgumentException(
String.format("The value must be between %s and %s.",
ManagementClientConstants.MIN_DUPLICATE_HISTORY_DURATION,
Expand Down
11 changes: 5 additions & 6 deletions ...s/src/main/java/com/microsoft/azure/servicebus/management/TopicDescriptionSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,20 +176,17 @@ private static TopicDescription parseFromEntry(Node xEntry) {
Node node = nList.item(i);
if (node.getNodeType() == Node.ELEMENT_NODE) {
Element element = (Element)node;
switch(element.getTagName())
{
switch (element.getTagName()) {
case "title":
td = new TopicDescription(element.getFirstChild().getNodeValue());
break;
case "content":
NodeList qdNodes = element.getFirstChild().getChildNodes();
for (int j = 0; j < qdNodes.getLength(); j++)
{
for (int j = 0; j < qdNodes.getLength(); j++) {
node = qdNodes.item(j);
if (node.getNodeType() == Node.ELEMENT_NODE) {
element = (Element) node;
switch (element.getTagName())
{
switch (element.getTagName()) {
case "MaxSizeInMegabytes":
td.maxSizeInMB = Long.parseLong(element.getFirstChild().getNodeValue());
break;
Expand Down Expand Up @@ -223,6 +220,8 @@ private static TopicDescription parseFromEntry(Node xEntry) {
case "SupportOrdering":
td.supportOrdering = Boolean.parseBoolean(element.getFirstChild().getNodeValue());
break;
default:
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
}
}
}
Expand Down
11 changes: 5 additions & 6 deletions ...s/src/main/java/com/microsoft/azure/servicebus/management/TopicRuntimeInfoSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,17 @@ private static TopicRuntimeInfo parseFromEntry(Node xEntry) {
Node node = nList.item(i);
if (node.getNodeType() == Node.ELEMENT_NODE) {
Element element = (Element)node;
switch(element.getTagName())
{
switch (element.getTagName()) {
case "title":
topicRuntimeInfo = new TopicRuntimeInfo(element.getFirstChild().getNodeValue());
break;
case "content":
NodeList qdNodes = element.getFirstChild().getChildNodes();
for (int j = 0; j < qdNodes.getLength(); j++)
{
for (int j = 0; j < qdNodes.getLength(); j++) {
node = qdNodes.item(j);
if (node.getNodeType() == Node.ELEMENT_NODE) {
element = (Element) node;
switch (element.getTagName())
{
switch (element.getTagName()) {
case "AccessedAt":
topicRuntimeInfo.setAccessedAt(Instant.parse(element.getFirstChild().getNodeValue()));
break;
Expand Down Expand Up @@ -105,6 +102,8 @@ private static TopicRuntimeInfo parseFromEntry(Node xEntry) {
case "TransferDeadLetterMessageCount":
topicRuntimeInfo.getMessageCountDetails().setTransferDeadLetterMessageCount(Long.parseLong(element.getFirstChild().getNodeValue()));
break;
default:
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
}
}
}
Expand Down
16 changes: 15 additions & 1 deletion ...e-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/ClientConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

package com.microsoft.azure.servicebus.primitives;

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.Properties;
import java.util.UUID;
Expand Down Expand Up @@ -190,12 +194,22 @@ private ClientConstants() { }
private static String getClientVersion() {
String clientVersion;
final Properties properties = new Properties();
InputStream clientPropInputStream = null;
try {
properties.load(ClientConstants.class.getResourceAsStream("/client.properties"));
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

clientPropInputStream = ClientConstants.class.getResourceAsStream("/client.properties");
properties.load(clientPropInputStream);
clientVersion = properties.getProperty("client.version");
} catch (Exception e) {
clientVersion = "NOTFOUND";
TRACE_LOGGER.error("Exception while retrieving client version. Exception: ", e.toString());
} finally {
if (clientPropInputStream != null) {
try {
clientPropInputStream.close();
Copy link
Contributor Author

@mssfang mssfang May 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close the clientPropInputStream

} catch (IOException e) {
TRACE_LOGGER.error("Client Properties InputStream doesn't close properly. Exception: ", e.toString());
}
}
}

return clientVersion;
Expand Down
9 changes: 5 additions & 4 deletions .../azure-servicebus/src/main/java/com/microsoft/azure/servicebus/primitives/Controller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.atomic.AtomicBoolean;

class Controller {
private static final Logger TRACE_LOGGER = LoggerFactory.getLogger(Controller.class);
private MessagingFactory messagingFactory;
private CoreMessageSender internalSender;
private boolean isInitialized = false;
private AtomicBoolean isInitialized = new AtomicBoolean(false);
private URI namespaceEndpointURI;
private ClientSettings clientSettings;

Expand All @@ -41,7 +42,7 @@ class Controller {
}

synchronized CompletableFuture<Void> initializeAsync() {
if (this.isInitialized) {
Copy link
Contributor Author

@mssfang mssfang May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

if (this.isInitialized.get()) {
return CompletableFuture.completedFuture(null);
} else {
TRACE_LOGGER.info("Creating MessageSender to coordinator");
Expand All @@ -54,7 +55,7 @@ synchronized CompletableFuture<Void> initializeAsync() {
senderFuture.handleAsync((s, coreSenderCreationEx) -> {
if (coreSenderCreationEx == null) {
this.internalSender = s;
this.isInitialized = true;
this.isInitialized.set(true);
TRACE_LOGGER.info("Created MessageSender to coordinator");
postSenderCreationFuture.complete(null);
} else {
Expand All @@ -72,7 +73,7 @@ synchronized CompletableFuture<Void> initializeAsync() {
public CompletableFuture<Binary> declareAsync() {
Message message = Message.Factory.create();
Declare declare = new Declare();
message.setBody(new AmqpValue(declare));
message.setBody(new AmqpValue(declare));

return this.internalSender.sendAndReturnDeliveryStateAsync(
message,
Expand Down
Loading