[ROCKETMQ-101]Fix possible NullPointerException when retry in send Async way

[Jaskey]

JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-101

[coveralls]

Coverage increased (+0.03%) to 31.549% when pulling a8802d0 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[coveralls]

Coverage increased (+0.09%) to 31.614% when pulling a8802d0 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[coveralls]

Coverage increased (+0.09%) to 31.614% when pulling a8802d0 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[coveralls]

Coverage increased (+0.09%) to 31.614% when pulling a8802d0 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[lizhanhui]

This issue is valid and the bug will be triggered under the following conditions:

1. A target message queue is specified, which will pass null for TopicPublishInfo parameter;
2. Async sending;
3. Retry happens;
4. sendLatencyFaultEnable is enabled(On default, it's not enabled)

The PR, IMO, is not good as it simply disabled promised retry attempts without notice. Instead of logging retry failure, retry sending should be done against the original chosen broker.

[Jaskey]

@lizhanhui , # 4 is not the nessary conditions, since even it is not enble, the tpInfo's method is still used.

    if (this.sendLatencyFaultEnable) {
        try {
            int index = tpInfo.getSendWhichQueue().getAndIncrement();
            for (int i = 0; i < tpInfo.getMessageQueueList().size(); i++) {
                int pos = Math.abs(index++) % tpInfo.getMessageQueueList().size();
                if (pos < 0)
                    pos = 0;
                MessageQueue mq = tpInfo.getMessageQueueList().get(pos);
                if (latencyFaultTolerance.isAvailable(mq.getBrokerName())) {
                    if (null == lastBrokerName || mq.getBrokerName().equals(lastBrokerName))
                        return mq;
                }
            }

            final String notBestBroker = latencyFaultTolerance.pickOneAtLeast();
            int writeQueueNums = tpInfo.getQueueIdByBroker(notBestBroker);
            if (writeQueueNums > 0) {
                final MessageQueue mq = tpInfo.selectOneMessageQueue();
                if (notBestBroker != null) {
                    mq.setBrokerName(notBestBroker);
                    mq.setQueueId(tpInfo.getSendWhichQueue().getAndIncrement() % writeQueueNums);
                }
                return mq;
            } else {
                latencyFaultTolerance.remove(notBestBroker);
            }
        } catch (Exception e) {
            log.error("Error occurred when selecting message queue", e);
        }

        return tpInfo.selectOneMessageQueue();
    }

    return tpInfo.selectOneMessageQueue(lastBrokerName);//thrown from here

Besides, if topic route info is null which propably means user is send through my seletor method, resend should still respect user's seletor, so the same chosen broker is not enough, the same chosen queue is needed too, which may be another issue.

I guess it is not a very minal effort, since the existing interface does not record any chosen queue info.

[lizhanhui]

Yes, #4 is not required as MQFaultStrategy#92 may also generate NPE.
Queue ID info is contained in the request header, minimal effort suffices.

[Jaskey]

@lizhanhui @zhouxinyu , please review the updated pr

[lizhanhui]

Looks good.

[coveralls]

Coverage increased (+0.02%) to 31.535% when pulling 18de299 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[zhouxinyu]

Looks good, please @vongosling help review.

[Jaskey]

@vongosling @lizhanhui @zhouxinyu

This bug is crucial and easy to reproduce which makes sending message with seletor is not reliable at all.

When can this pr be merged and release the fix version?

[lizhanhui]

@Jaskey You are right, I'll follow up and merge this PR ASAP.

[vongosling]

LGTM~

[lizhanhui]

@Jaskey I'll merge this PR soon. Please close this issue.

[Jaskey]

Thank you @vongosling @lizhanhui @zhouxinyu

[vongosling]

@lizhanhui @Jaskey we'd better close after merge :-)

[coveralls]

Coverage decreased (-0.5%) to 31.043% when pulling 18de299 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[coveralls]

Coverage decreased (-0.5%) to 31.043% when pulling 18de299 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[coveralls]

Coverage decreased (-0.5%) to 31.043% when pulling 18de299 on Jaskey:ROCKETMQ-101-NPE-when-retry into 573b22c on apache:master.

[lizhanhui]

Merged