activemq-nms-amqp icon indicating copy to clipboard operation
activemq-nms-amqp copied to clipboard

AMQNET-625: Content Property of IBytesMessage should be idempotent

Open Havret opened this issue 4 years ago • 6 comments

Content Property getter should not generate side effects.

@cjwmorgan-sol could you please take a look at it?

Havret avatar Nov 15 '19 22:11 Havret

Can you add a test where when byte message is 24 bytes long, and three read calls with a dest byte array is size 8 bytes. Expected out come is first read returns first 8 bytes aka bytes in position 0-7, second read returns bytes position 8-13 , and last read returns bytes in position 14 to 23. Test should include a further read which obviously would not return data as no further bytes to be read unless message is reset

michaelandrepearce avatar Nov 16 '19 02:11 michaelandrepearce

@michaelandrepearce Done. Moreover I've changed slightly the name of the first test to make it more descriptive.

The other thing is that I am not sure what should be the expected behavior of Content setter when somebody starts messing with Write* methods. Content property is a peculiar thing for NMS IBytesMessage and there is no equivalent of it in JMS API, thus it would be a futile attempt to look for inspiration in qpid-jms, as I used to do to tackle previous problems.

My implementation mimics what was done in NMS MSMQ:

https://github.com/apache/activemq-nms-msmq/blob/b6d954d9a97f11459f1b34956aeae675e456dfdd/src/main/csharp/BaseMessage.cs#L42-L47

where Content Property circumvents all the bits related to Binary Writers and Readers and exposes direct access to underlying message body.

@cjwmorgan-sol, @gemmellr, @michaelandrepearce What do you think?

Havret avatar Nov 17 '19 13:11 Havret

Currently modifying the return byte array reference modifies the underlying message byte array from message.Content is this desired? Or are you looking for a block copy of the Content when using get? And set?

cjwmorgan-sol avatar Nov 18 '19 20:11 cjwmorgan-sol

@cjwmorgan-sol You are correct. I do return direct reference. TBH I don't know what is the expected behavior. Even if we allow mutating the underlying message representation, user always operates on deep copy of the message.

Havret avatar Nov 18 '19 20:11 Havret

Sounds good to me. It might be worth adding a comment describing the behaviour implemented for Content set and get for bytes messages though. Where get allows read write access to the buffer's bytes and set allows modifying the Buffer (ie size), or something along those lines. Also that "user always operates on deep copy of the message". Otherwise developers have to another layer deep (amqpnetlite) to find out the byte array is safe.

cjwmorgan-sol avatar Nov 18 '19 21:11 cjwmorgan-sol

So a message can only ever be in write or read mode. There are details in the spec around this and some tests that exist. Ill try dig it out and pass to you offline

michaelandrepearce avatar Nov 18 '19 22:11 michaelandrepearce