[ARTEMIS-6129] If a empty foreign MapMessage is send, map encoding is skipped

[davidvoit]

Body would be 0 bytes and reading such a message with the broker would directly send it to the DLQ

WARN  [org.apache.activemq.artemis.protocol.amqp.logger] AMQ111005: Failed to convert message. Sending it to Dead Letter Address.
org.apache.activemq.artemis.protocol.amqp.converter.coreWrapper.ConversionException: readerIndex(0) + length(1) exceeds writerIndex(0): ReadOnlyByteBuf(ridx: 0, widx: 0, cap: 0/0, unwrapped: UnpooledSlicedByteBuf(ridx: 0, widx: 0, cap: 0/0, unwrapped: UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 558, cap: 558)))
        at org.apache.activemq.artemis.protocol.amqp.converter.CoreAmqpConverter.fromCore(CoreAmqpConverter.java:320)
        at org.apache.activemq.artemis.protocol.amqp.converter.CoreAmqpConverter.checkAMQP(CoreAmqpConverter.java:79)
        at org.apache.activemq.artemis.protocol.amqp.proton.AMQPMessageWriter.writeBytes(AMQPMessageWriter.java:61)
        at org.apache.activemq.artemis.protocol.amqp.proton.MessageWriter.accept(MessageWriter.java:39)
        at org.apache.activemq.artemis.protocol.amqp.proton.MessageWriter.accept(MessageWriter.java:28)
        at org.apache.activemq.artemis.core.server.impl.MessageReferenceImpl.run(MessageReferenceImpl.java:138)
        at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
        at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
        at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)
        at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:405)
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:998)
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:120)
Caused by: java.lang.IndexOutOfBoundsException: readerIndex(0) + length(1) exceeds writerIndex(0): ReadOnlyByteBuf(ridx: 0, widx: 0, cap: 0/0, unwrapped: UnpooledSlicedByteBuf(ridx: 0, widx: 0, cap: 0/0, unwrapped: UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 558, cap: 558)))
        at io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1442)
        at io.netty.buffer.AbstractByteBuf.readByte(AbstractByteBuf.java:730)
        at io.netty.buffer.WrappedByteBuf.readByte(WrappedByteBuf.java:529)
        at org.apache.activemq.artemis.utils.collections.TypedProperties.decode(TypedProperties.java:573)
        at org.apache.activemq.artemis.utils.collections.TypedProperties.decode(TypedProperties.java:659)
        at org.apache.activemq.artemis.reader.MapMessageUtil.readBodyMap(MapMessageUtil.java:46)
        at org.apache.activemq.artemis.protocol.amqp.converter.coreWrapper.CoreMapMessageWrapper.decode(CoreMapMessageWrapper.java:220)
        at org.apache.activemq.artemis.protocol.amqp.converter.CoreAmqpConverter.fromCore(CoreAmqpConverter.java:97)
        ... 12 more

[clebertsuconic]

can you provide a test? a test in integration-tests would be great.

It's not difficult to write one, and you can even ask ChatGPT to help you. just to give it a more compreensive view of how it would fail.

[davidvoit]

Sure. Background for this bug is that we migrating from our old JMS provider to artemis. We are using the JMSBridge. If we recieve a JMS MapMessage from the old provider and forward it using the bridge, it will now be 0 bytes long in the body.

If a client is recieving this message it will trigger the exception as show in the pull request text.

Would a unit test be enough which shows that the body is not encoded?

[davidvoit]

I see that all the "fun" convenion functions like SimpleJMSMapMessage are not in the test directory of artemis-jms-client.

I would have expected a test something like this:

   @Test
   public void testSerializeEmptyForeignMapMessage() throws Exception {
      ClientMessage clientMessage = new ClientMessageImpl(ActiveMQTextMessage.TYPE, true, 0, System.currentTimeMillis(), (byte) 4, 1000);
      ClientSession session = new FakeSession(clientMessage);

      MapMessage emptyForeignMessage = new SimpleJMSMapMessage();

      ActiveMQMessage copy = new ActiveMQMapMessage(emptyForeignMessage, null);

      copy.doBeforeSend();

      Assertions.assertEquals(4, copy.getCoreMessage().getBodySize());
   }

Where would be a good location for such a test?

[davidvoit]

Ahhh I see you have a dedicated jms-test sub maven project - sorry the code is new to me.
Let me work on something here

[jbertram]

I went ahead and opened https://issues.apache.org/jira/browse/ARTEMIS-6129 for this. Please reference ARTEMIS-6129 in your commit message as detailed here. Thanks!

[davidvoit]

Unit tests added. It's fails without the change and works with.

From me issue I expect you also want to fix the broker that it can handle empty body?
This would be something which is too big of a task for me, but hopefully this client side change is a good start :-)

[davidvoit]

I reformated FakeSession it now to 3 spaces, hope this makes the checkstyle check happy now

[davidvoit]

FakeSession is now removed, just using Mockito

[davidvoit]

mockito should also now run as an javaagent. The unit test worked while running under intellij, currently running with -Ptests under maven.

I hope now I'm done with all needed changes

[davidvoit]

Could you approve the workflow again?

[davidvoit]

I could split the commit into two if needed (1. enabling mockito support and removing FakeSession 2. the actual problem with foreign MapMessages) - just let me know if you need this change

Otherwise from my point of view this is ready to merge

[davidvoit]

Oh I see i removed the ${skipJmsTests} by error. Let me fix this

[jbertram]

I might be missing something, but I don't think the pom.xml needs the maven-dependency-plugin or argLine changes. I'm able to run the test both from the command-line and IDE (i.e. IntelliJ) without them, e.g. running this from tests/jms-tests:

mvn -Ptests -DfailIfNoTests=false -Dtest=MessageHeaderTest test

Also, the changes in org.apache.activemq.artemis.jms.tests.message.MessageHeaderTest can be simplified quite a bit by using a single method, e.g.:

   private static ClientSession createMockSession(ClientMessage clientMessage) {
      ClientSession session = Mockito.mock(ClientSession.class);
      Mockito.when(session.createMessage(
            Mockito.anyByte(),
            Mockito.anyBoolean(),
            Mockito.anyLong(),
            Mockito.anyLong(),
            Mockito.anyByte()))
         .thenReturn(clientMessage);
      return session;
   }

And calling this method in tests which need it.

[davidvoit]

mockito is complaining that it won't run in future without the agent. The argline was copied from existing code. yes it's working without it - I just was handline the "runtime installed agents are deprecated and wont work in feature versions" part - or my understanding of it.

Should I remove it?

Sure I can create a convenioned functions in message header - this was mostly make the test "self-complete" thing.
Will use a helper functions like requested today

[davidvoit]

Mockito is currently self-attaching to enable the inline-mock-maker. This will no longer work in future releases of the JDK. Please add Mockito as an agent to your build as described in Mockito's documentation: https://javadoc.io/doc/org.mockito/mockito-core/latest/org.mockito/org/mockito/Mockito.html#0.3

This was the warning btw

[davidvoit]

createMockSession helper <- Done