[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/2010


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r184701274
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ throw new ActiveMQNullRefException("Reference to message is 
null");
--- End diff --

The method I saw didn't have a throw.. I'm not sure what happened to github 
diffs here.


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r184601120
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ throw new ActiveMQNullRefException("Reference to message is 
null");
--- End diff --

ActiveMQNullRefException calls super(ActiveMQExceptionType.NULL_REF), which 
as I understand invokes NULL_REF.createException.


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r184600627
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ NULL_REF.createException("Reference to message is null");
--- End diff --

Yeah, i forgot to throw that. I am throwing ActiveMQNullRefException, which 
under the hood does super(ActiveMQExceptionType.NULL_REF). Is that ok?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r184572931
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ NULL_REF.createException("Reference to message is null");
--- End diff --

@stanlyDoge what was your intention here? this last expression here is 
pretty much dead code.. you are creating an exception and doing nothing with 
it... the previous logging would work.. but the exception here is dead code.


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r183418125
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2714,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ NULL_REF.createException("Reference to message is null");
--- End diff --

didn't you miss throw NULL_REF here?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r181386656
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2713,11 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ ActiveMQServerLogger.LOGGER.nullRefMessage();
+ throw new ActiveMQNullRefException("Reference to message is 
null");
--- End diff --

Was this an accident? WEren't you supposed to use NULL_REF.createException 
here?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180976916
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2727,7 +2731,7 @@ private Message makeCopy(final MessageReference ref,
   Message copy = message.copy(newID);
 
   if (copyOriginalHeaders) {
- copy.referenceOriginalMessage(message, ref != null ? 
ref.getQueue().getName().toString() : null);
+ copy.referenceOriginalMessage(message, 
ref.getQueue().getName().toString());
--- End diff --

@clebertsuconic thank you :) Can you please check again? Especially null 
reference logging. 


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180971622
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java
 ---
@@ -511,22 +511,24 @@ public void close() {
public void transferConnection(final CoreRemotingConnection 
newConnection) {
   // Needs to synchronize on the connection to make sure no packets 
from
   // the old connection get processed after transfer has occurred
-  synchronized (connection.getTransferLock()) {
- connection.removeChannel(id);
+  if (connection != null) {
--- End diff --

Well, it is null-checked few lines bellow so it confued me.


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859972
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2727,7 +2731,7 @@ private Message makeCopy(final MessageReference ref,
   Message copy = message.copy(newID);
 
   if (copyOriginalHeaders) {
- copy.referenceOriginalMessage(message, ref != null ? 
ref.getQueue().getName().toString() : null);
+ copy.referenceOriginalMessage(message, 
ref.getQueue().getName().toString());
--- End diff --

keep it ! :) nice one!


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859106
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java
 ---
@@ -205,7 +205,7 @@ public PageSubscriptionCounter getCounter() {
 */
@Override
public boolean reloadPageCompletion(PagePosition position) throws 
Exception {
-  if (!pageStore.checkPageFileExists((int)position.getPageNr())) {
+  if (pageStore != null && 
!pageStore.checkPageFileExists((int)position.getPageNr())) {
--- End diff --

pageStore is final.. it will never be null


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180859389
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
 ---
@@ -2712,6 +2712,10 @@ private Message makeCopy(final MessageReference ref, 
final boolean expiry) throw
private Message makeCopy(final MessageReference ref,
 final boolean expiry,
 final boolean copyOriginalHeaders) throws 
Exception {
+  if (ref == null) {
+ return null;
--- End diff --

I would actually throw an Exception.. you're not supposed to make a copy of 
a ref == null.

it's a valid assertion, but I would actually make it throw 
NullPointerException  here. (Or a chosen exception).


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180858486
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java
 ---
@@ -511,22 +511,24 @@ public void close() {
public void transferConnection(final CoreRemotingConnection 
newConnection) {
   // Needs to synchronize on the connection to make sure no packets 
from
   // the old connection get processed after transfer has occurred
-  synchronized (connection.getTransferLock()) {
- connection.removeChannel(id);
+  if (connection != null) {
--- End diff --

same thing here.. how connection would be null?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180858361
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java
 ---
@@ -380,7 +380,7 @@ public Packet sendBlocking(final Packet packet,
 
 connection.getTransportConnection().write(buffer, false, 
false);
 
-long toWait = connection.getBlockingCallTimeout();
--- End diff --

Looking at the code, I can't see how connection will ever be null here..  I 
think you should remove this one...

For instance... the very same connection was used before within a lock.. 
what would issue a NPE a lot earlier than here.

I don't see how connection could ever be null.


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180654379
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
 ---
@@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
  throw 
ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, 
info.getName().toString(), info.getRoutingTypes());
   }
 
-  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
+  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo
 == null ? RoutingType.ANYCAST 
:addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
--- End diff --

@jdanekrh Good catch!


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[jdanekrh]

Github user jdanekrh commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180652792
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
 ---
@@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
  throw 
ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, 
info.getName().toString(), info.getRoutingTypes());
   }
 
-  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
+  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo
 == null ? RoutingType.ANYCAST 
:addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
--- End diff --

About 20 lines above this one is

RoutingType rt = (routingType == null ? 
ActiveMQDefaultConfiguration.getDefaultRoutingType() : routingType);


[here](https://github.com/apache/activemq-artemis/blob/fa3e37fdb91c285bcfe87cab3d771dfe80019e18/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java#L2754)
 and 
[here](https://github.com/apache/activemq-artemis/blob/fa3e37fdb91c285bcfe87cab3d771dfe80019e18/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java#L2865)

So maybe that one?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

Github user stanlyDoge commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2010#discussion_r180649567
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java
 ---
@@ -2769,7 +2769,7 @@ public Queue createQueue(final AddressInfo addrInfo,
  throw 
ActiveMQMessageBundle.BUNDLE.invalidRoutingTypeForAddress(rt, 
info.getName().toString(), info.getRoutingTypes());
   }
 
-  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
+  final QueueConfig queueConfig = 
queueConfigBuilder.filter(filter).pagingManager(pagingManager).user(user).durable(durable).temporary(temporary).autoCreated(autoCreated).routingType(addrInfo
 == null ? RoutingType.ANYCAST 
:addrInfo.getRoutingType()).maxConsumers(maxConsumers).purgeOnNoConsumers(purgeOnNoConsumers).exclusive(exclusive).lastValue(lastValue).build();
--- End diff --

What should be default routing type?


---

[GitHub] activemq-artemis pull request #2010: ARTEMIS-1801 removing null-unchecked de...

[stanlyDoge]

GitHub user stanlyDoge opened a pull request:

https://github.com/apache/activemq-artemis/pull/2010

ARTEMIS-1801 removing null-unchecked dereferences

There were some cases where value was checked for null and later directly 
dereferenced without check. I added checks. I am not sure about "default 
behaviour" tho. Can anybody check?

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stanlyDoge/activemq-artemis ARTEMIS-1801

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2010.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2010


commit 8f393828473c75321b28a4fe63f5f08fb7006bbe
Author: Stanislav Knot 
Date:   2018-04-11T06:43:07Z

ARTEMIS-1801 removing null-unchecked dereferences




---