Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,9 @@ public void execute() throws Exception {
}
});
}
if (m.propertyExists(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY)) {
m.setJMSDeliveryTime(m.getLongProperty(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY));
Copy link
Member

Choose a reason for hiding this comment

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

Here, JMSDeliveryTime is restored from a message property.
This needs verification to ensure the consumer-side restoration isn't overwritten.

}
return m;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import jakarta.jms.JMSException;
import jakarta.jms.Message;

import org.apache.activemq.command.ActiveMQMessage;
import org.apache.activemq.command.ActiveMQDestination;
import org.apache.activemq.command.ProducerAck;
import org.apache.activemq.command.ProducerId;
Expand Down Expand Up @@ -326,6 +327,13 @@ public void send(Destination destination, Message message, int deliveryMode, int
}
}

long delay = getDeliveryDelay();
if (delay > 0) {
message.setLongProperty("AMQ_SCHEDULED_DELAY", delay);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the constante like bellow ?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, to be consistent with the other parts of this class, we should use a constant here.

long deliveryTime = System.currentTimeMillis() + delay;
message.setLongProperty(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY, deliveryTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

To be checked with ActiveMQSession which sets to now without delay, correct?

Copy link
Member

Choose a reason for hiding this comment

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

We have something weird here. As AMQ_SCHEDULED_DELAY and JMSDeliveryTime properties are set in both ActiveMQProducer.send() and ActiveMQMessageProducer.send().

As ActiveMQProducer.send() delegates to ActiveMQMessageProducer.send(), the properties get set twice with potentially different System.currentTimeMillis() values.

Imho, the logic should be in ActiveMQMessageProducer.send() only.

}

this.session.send(this, dest, message, deliveryMode, priority, timeToLive, disableMessageID, disableMessageTimestamp, producerWindow, sendTimeout, onComplete);

stats.onMessage();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public abstract class ActiveMQMessageProducerSupport implements MessageProducer,
protected long defaultTimeToLive;
protected int sendTimeout=0;

private long deliveryDelay = 0;

public ActiveMQMessageProducerSupport(ActiveMQSession session) {
this.session = session;
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
Expand All @@ -56,7 +58,12 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
*/
@Override
public void setDeliveryDelay(long deliveryDelay) throws JMSException {
throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
checkClosed();
// This should now compile after the rebase!
Copy link
Contributor

Choose a reason for hiding this comment

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

If it does eventually compile, please remove this comment ;-)

if (deliveryDelay < 0 && session.connection.isStrictCompliance()) {
Copy link
Member

Choose a reason for hiding this comment

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

The check is correct to validate the parameter.
However, the validation only rejects negative values when strictCompliance is true.
When strictCompliance is false, negative delays are silently accepted but will produce nonsensical AMQ_SCHEDULED_DELAY values (negative scheduling).

I suggest:

  1. Always rejecting negative values (JMS spec says delay must be >= 0)
    or
  2. Clamping to 0

throw new jakarta.jms.JMSException("Delivery delay cannot be negative.");
}
this.deliveryDelay = deliveryDelay;
}

/**
Expand All @@ -68,7 +75,8 @@ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
*/
@Override
public long getDeliveryDelay() throws JMSException {
throw new UnsupportedOperationException("getDeliveryDelay() is not supported");
checkClosed();
return this.deliveryDelay;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import jakarta.jms.ObjectMessage;
import jakarta.jms.TextMessage;

import org.apache.activemq.command.ActiveMQMessage;
import org.apache.activemq.util.JMSExceptionSupport;
import org.apache.activemq.util.TypeConversionSupport;

Expand All @@ -46,6 +47,7 @@ public class ActiveMQProducer implements JMSProducer {
// QoS override of defaults on a per-JMSProducer instance basis
private String correlationId = null;
private byte[] correlationIdBytes = null;
private Long deliveryDelay = null;
private Integer deliveryMode = null;
private Boolean disableMessageID = false;
private Boolean disableMessageTimestamp = false;
Expand Down Expand Up @@ -90,6 +92,13 @@ public JMSProducer send(Destination destination, Message message) {
}
}

// [AMQ-8320] Producer setting for deliveryDelay will override user-specified ActiveMQ Scheduled Delay property
if(this.deliveryDelay != null) {
long deliveryTimeMillis = System.currentTimeMillis() + this.deliveryDelay;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that is really necessary here.
For JMS 2+, the path is ActiveMQProducer.send() → ActiveMQMessageProducer.send() → ActiveMQSession.send().

So applying on ActiveMQMessageProducer should cover both JMS 1 and 2+
For now, we set a value here and we override it with a different value in ActiveMQMessageProducer.

Not a big deal functionally, but having the logic duplicated will make maintenance harder in the future and may introduce discrepancies.

message.setLongProperty(ScheduledMessage.AMQ_SCHEDULED_DELAY, this.deliveryDelay);
message.setLongProperty(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY, deliveryTimeMillis);
}

activemqMessageProducer.send(destination, message, getDeliveryMode(), getPriority(), getTimeToLive(), getDisableMessageID(), getDisableMessageTimestamp(), null);
} catch (JMSException e) {
throw JMSExceptionSupport.convertToJMSRuntimeException(e);
Expand Down Expand Up @@ -246,12 +255,23 @@ public long getTimeToLive() {

@Override
public JMSProducer setDeliveryDelay(long deliveryDelay) {
throw new UnsupportedOperationException("setDeliveryDelay(long) is not supported");
try {
// Tell the internal core producer about the delay
this.activemqMessageProducer.setDeliveryDelay(deliveryDelay);

// Update the local field in this wrapper for consistency
this.deliveryDelay = deliveryDelay;

} catch (JMSException e) {
// JMS 2.0 requires converting checked exceptions to RuntimeExceptions
throw JMSExceptionSupport.convertToJMSRuntimeException(e);
}
return this;
}

@Override
public long getDeliveryDelay() {
throw new UnsupportedOperationException("getDeliveryDelay() is not supported");
return this.deliveryDelay;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be implemented in a similar way as getTimeToLive() or getPriority() to avoid the NPE because of the unboxing of the Long?

Copy link
Member

Choose a reason for hiding this comment

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

That's correct and a blocker to me.

As this class declares private Long deliveryDelay = null (boxed), here you do auto-unboxes to long.
When deliveryDelay is null (the default), this will throw NullPointerException.

As @jeanouii said, you have to use a similar approach as in getTimeToLive() or getPriority() methods.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class ActiveMQMessage extends Message implements org.apache.activemq.Mess
public static final byte DATA_STRUCTURE_TYPE = CommandTypes.ACTIVEMQ_MESSAGE;
public static final String DLQ_DELIVERY_FAILURE_CAUSE_PROPERTY = "dlqDeliveryFailureCause";
public static final String BROKER_PATH_PROPERTY = "JMSActiveMQBrokerPath";
public static final String JMS_DELIVERY_TIME_PROPERTY = "JMSDeliveryTime";

private static final Map<String, PropertySetter> JMS_PROPERTY_SETERS = new HashMap<String, PropertySetter>();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.activemq;

import jakarta.jms.Connection;
import jakarta.jms.MessageProducer;
import jakarta.jms.Session;
import org.apache.activemq.command.ActiveMQMessage;
import org.junit.Test;

import static org.apache.activemq.command.DataStructureTestSupport.assertEquals;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong one??

Copy link
Member

Choose a reason for hiding this comment

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

I think so 😄

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

public class ActiveMQDeliveryDelayTest {

private final String connectionUri = "vm://localhost?broker.persistent=false";

@Test
public void testStrictComplianceRejectsNegativeDelay() throws Exception {
ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory(connectionUri);
// Turn ON strict compliance (Jakarta 3.1 requirement)
factory.setStrictCompliance(true);

try (Connection conn = factory.createConnection();
Session sess = conn.createSession(false, Session.AUTO_ACKNOWLEDGE)) {

MessageProducer producer = sess.createProducer(sess.createQueue("TEST.STRICT"));

try {
producer.setDeliveryDelay(-1000L);
fail("Should have thrown a JMSException for negative delay in strict mode");
} catch (jakarta.jms.JMSException e) {
// Success: Exception was thrown as required by the spec
}
}
}

@Test
public void testLegacyBehaviorAllowsNegativeDelay() throws Exception {
ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory(connectionUri);
// Turn OFF strict compliance (Legacy ActiveMQ behavior)
factory.setStrictCompliance(false);

try (Connection conn = factory.createConnection();
Session sess = conn.createSession(false, Session.AUTO_ACKNOWLEDGE)) {

MessageProducer producer = sess.createProducer(sess.createQueue("TEST.LEGACY"));

// Should NOT throw an exception
producer.setDeliveryDelay(-1000L);
assertEquals(-1000L, producer.getDeliveryDelay());
}
}

@Test
public void testDeliveryDelayEffectiveOnMessage() throws Exception {
ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory(connectionUri);
try (Connection conn = factory.createConnection();
Session sess = conn.createSession(false, Session.AUTO_ACKNOWLEDGE)) {

MessageProducer producer = sess.createProducer(sess.createQueue("TEST.EFFECTIVE"));
long delay = 5000L;
producer.setDeliveryDelay(delay);

ActiveMQMessage msg = (ActiveMQMessage) sess.createTextMessage("Hello");
producer.send(msg);

// Verify Broker-side scheduling property
assertEquals("Broker delay property missing",
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the session comment above, if you also check the header instead of the custom property, it might fail?

To be checked

delay, msg.getLongProperty("AMQ_SCHEDULED_DELAY"));

// Verify Consumer-side visibility property (matching #1157 logic)
assertTrue("JMSDeliveryTime property missing or incorrect",
msg.getLongProperty(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY) >= System.currentTimeMillis() + delay - 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the CI, this might randomly fail under load or when parallelism is used.

You could do something like this instead

final  long before = System.currentTimeMillis();
  producer.send(msg);

...

  long deliveryTime = msg.getLongProperty(ActiveMQMessage.JMS_DELIVERY_TIME_PROPERTY);
  assertTrue(deliveryTime >= before + delay);
  assertTrue(deliveryTime <= System.currentTimeMillis() + delay);

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it smells flakiness here 😄

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,16 @@ public void testSessionSharedDurableConsumerSelector() throws JMSException {
session.createSharedDurableConsumer(session.createTopic("test"), null, null);
}

@Test(expected = UnsupportedOperationException.class)
@Test
public void testProducerDeliveryDelayGet() throws JMSException {
messageProducer.getDeliveryDelay();
assertEquals(0, messageProducer.getDeliveryDelay());
}

@Test(expected = UnsupportedOperationException.class)
@Test
public void testProducerDeliveryDelaySet() throws JMSException {
messageProducer.setDeliveryDelay(1000l);
long delay = 1000L;
messageProducer.setDeliveryDelay(delay);
assertEquals(delay, messageProducer.getDeliveryDelay());
}

@Test(expected = UnsupportedOperationException.class)
Expand Down