hreinhardt/amqp

Should return published message's sequence number when in confirm mode

Opened this issue · 8 comments

dbeacham commented

Either getting publishMsg to return it or via a new wrapping method as it would be a breaking change to the API.

Not being able to match the outgoing message with the delivery-tag in the addConfirmationListener method prevents additional, message specific logic from being performed. e.g. to mark messages as having been published in an external system.

hreinhardt commented

I have changed publishMsg to return the sequence number: 333e8d4

I'm not a big fan of breaking changes, but in this case it seems unlikely to break a lot of code and should be easy to fix.

Let me know if it works for you.

dbeacham commented

That's working well for me, thanks!

It might also be worth a mention in the docs that channels in confirm mode aren't thread safe.

hreinhardt commented

Do you mean that the publishMsg method isn't thread-safe or is there something else?

dbeacham commented

I don't think publishMsg can be used in a thread safe manner in confirm mode.

I think this is possible:

  1. msgA published by thread 1, so rabbit marks it as seqNum 1
  2. msgB published by thread 2 ,so rabbit marks it as seqNum 2
  3. thread 2 gets to publishMsg nextSeqNum logic first so channel incorrectly associates msgB as seqNum 1
  4. thread 1 gets to publishMsg nextSeqNum logic and incorrectly associates msgB as seqNum 1

It's not a problem if you're not tracking which messages have been confirmed, but then I don't know why you'd be in confirm mode.

hreinhardt commented

I think it could be made thread-safe if there was a lock around writeAssembly and the nextSeqNum logic. Would it help you if it was thread-safe or are you only using one thread anyway?

dbeacham commented

I'm only using one thread - just thought I'd bring it to your attention - but an MVar would fix it.

An alternative might be to have publishMsg as a property on a channel and then have openChannel :: Connection -> ChannelMode -> IO Channel make you choose the mode of channel up front. The benefits being

  1. I don't think it makes sense to open up a channel in 'normal' mode, use it for a while, and then try to switch to transaction or confirm mode.
  2. you wouldn't be stuck with the confirm mode logic - and the need for locking - when in normal/transaction mode.
  3. it would be easily extended to other channel modes (although I'm not aware of any).

Not expecting to see this implemented here - although I might have a play around with this myself and report back.

hreinhardt commented

I updated the implementation to be thread-safe.

Regarding your suggestion, I'm not sure it would be worth the risk of breaking backwards-compatibility. The biggest benefit would be slightly improved performance. But there are other ways we could improve the performance of publishMsg, for example offering a function that allows for bulk-publishing of many messages.

dbeacham commented

I totally agree about backwards compatibility - was just thinking out loud.

I'll have a play with the new threadsafe fix over the weekend and let you know how it goes.