JBMESSAGING-1460 - ScheduledDeliveryTest on JBM 1.4

[clebert.suconic]

Howard,

testRemoveAllReferences was added on JBM 1.4 Branch.

//https://jira.jboss.org/jira/browse/JBMESSAGING-1460
 public void testRemoveAllReferences() throws Exception
 {

....
 assertRemainingMessages(-1); // (Why?)

....
 }

Why assertRemainingMessages for -1 and not 0?

The following test after this is failing, as setup aways check for remainingMessages = 0.


I'm adding the same check on setUp, so it is more clear the test is failing.

I'm reopening the JIRA as the test is failing.

[clebert.suconic]

BTW: I would fix it myself, but I was a little puzzled to why you are testing for the -1.

[clebert.suconic]

deliveringCount.decrement is being called twice.


I did some debugging and deliveringCount became negative at acknowledgeInternal. (I added some System.out to debug it)

protected void acknowledgeInternal(Delivery d, Transaction tx, boolean persist) throws Exception
 {

....
 if (!d.isRecovered())
 {
 if (deliveringCount.decrement() < 0)
 {
 System.out.println("Oops... deliveringCount became negative at acknowledgeInternal"); // I added this print here
 }
 }

......
 }

Why the test is asserting for -1? it seems to me the valid condition would be 0, and deliveringCount being decremented to -1 would be a bug?

[gaohoward]

Yes this is a bug. The 1460 fix doesn't handle the count correctly.
look at the method

public void removeAllReferences() throws Throwable
{
synchronized (lock)
{
.....//removing all non scheduled msg from memory.
deliveringCount.set(0);
}
//clear sch
clearAllScheduledDeliveries(true);

So the count will go minus. I should have put it before the
deliveringCount.set(0);
and the count should be OK.

Sorry, I'll correct it.

[timfox]

Howard-

Please **NEVER** hack tests to pass when there is a bug in the system (assertRemainingMessages(-1)) - this is extremely bad practice.

The test should be kept failing until the core issue is fixed.

-1 is clearly not a correct values for remaining messages.

Fudging tests is a sackable offence ;)

[timfox]

What JIRA is this change for?

[gaohoward]

Sorry Tim, I think I was too eager to get the test passed then.

The JIRA is to fix the removeAllReferences() issue where the scheduled message not got cleared.

[gaohoward]

Hi Clebert, there is a test failure on my local machine:

org.jboss.test.messaging.jms.ConnectionConsumerTest.testStopWhileProcessing()

the code is here:

public void testStopWhileProcessing() throws Exception
{
if (ServerManagement.isRemote()) return;

Connection connConsumer = null;

try
{
// some test, not pasted here.

ServerManagement.stop();
connConsumer.close();
connConsumer = null;
}
finally
{
if (connConsumer != null) connConsumer.close();
}
}

The failure message is:


Server 0 has not been started!
java.lang.Exception: Server 0 has not been started!
at org.jboss.test.messaging.tools.ServerManagement.insureStarted(ServerManagement.java:1262)
at org.jboss.test.messaging.tools.ServerManagement.getAttribute(ServerManagement.java:633)
at org.jboss.test.messaging.tools.ServerManagement.getAttribute(ServerManagement.java:628)
at org.jboss.test.messaging.MessagingTestCase.checkEmpty(MessagingTestCase.java:137)
at org.jboss.test.messaging.jms.JMSTestCase.checkNotEmpty(JMSTestCase.java:101)
at org.jboss.test.messaging.jms.JMSTestCase.tearDown(JMSTestCase.java:131)


You can see in the test the

ServerManagement.stop(); //stops server 0

is called in the end. But in tearDown() it checks to ensure the server is in started state. Which will fail. Any idea?

[gaohoward]

Yes, I found the reason. The check is added by you to avoid message count failures today. I'll change this special test.

[gaohoward]

I just move the check as follows (JMSTestCase.tearDown())


if (ServerManagement.isStarted(0))
{
if (checkNoMessageData())
{
fail("Message Data exists");
}

checkNotEmpty(); //moved from outside the block here.
}