Publisher Confirms and thread safety on Channel.

[moysesb]

I'm working on adding support for publisher confirms to amqp. In principle it is possible to implement this feature and still preserve Channel's thread safety, but that seems to me to be a unnecessary compromise given that no one should be using a channel from multiple threads anyway.

Would it be ok to make a Channels not thread-safe when the publisher confirm mode is selected?

[hreinhardt]

Could you explain why thread-safety would be affected?

[moysesb]

1. I would like to use an IORef in data Channel to hold the Int counter that is used to track published messages and not have to acquire a lock to increment it every time a message is published. If I use atomicModifyIORef or an MVar to preserve thread-safety then I take a performance hit.
2. Ids for messages that have been published but not yet confirmed yet must be tracked (I'm using Data.IntSet for this). If multiple threads are waiting on this set I'd have to use something like an unbounded list of TVar to wake them up when 'confirms' arrive. I'd prefer to have only only waiter to wake up ;-)

[hreinhardt]

Regarding your first point, my understanding is that atomicModifyIORef should be pretty fast. Considering that the whole amqp library hasn't really been optimized for performance I think it's very unlikely to be a bottleneck.

Now concerning your second point: If I understand you correctly, you want to allow users to only specify a single listener (which would probably be stored in an IORef). I think that is a good idea, as I can't think of a situation where users would want to have multiple listeners.

[moysesb]

I agree that atomicModifyIORef is fast enough given that what follows is a network operation. The issue is that if thread-safety is explicitly abandoned then there's no need for that lock (and we could possibly start looking at other areas where performance could be improved).

About the listeners, there are two separate issues there. One is the list of confirm listener "callbacks" (analogous to return and close listeners that exist today). That has to be protected with an MVar because reads and writes on that list are inherently concurrent.

The second issue is how to implement a waitForConfirms function on a Channel. If multiple threads can issue a call to that function concurrently, then when a basic_ack arrives, the channel receptor thread must notify all the threads blocked in waitForConfirms. I would like the channel receptor thread to wake up only one thread when a basic_ack arrives (and be explicit about it in the documentation).

[moysesb]

Another issue with multiple threads publishing on a single Channel with confirms selected is the semantics of the whole thing. If one thread asks to waitForConfirms, I would think it expects to be waken up when the messages it has published itself are confirmed, independently of what other threads may have published on the same Channel. So preserving channel semantics in the presence of publisher confirms would require the channel to keep track of each unconfirmed set independently (a kind of multiplexing of the multiplexing, aaargh).

[hreinhardt]

I would be fine with waitForConfirms only being supported when called from a single thread.

But I wonder if we need to implement waitForConfirms at all, given that the user could probably implement it himself using addConfirmationListener and some additional method to query the last published message id.

[moysesb]

I have client code waiting on the wings that would exercise both paths of that API, so I'd say it's a good idea to offer both a) a waitForConfirms function for clients/users who'd rather remain ignorant of the mechanics of how confirmations are implemented and b) a more fine grained interface via addConfirmationListener and access to the unconfirmed set, as you said, for those who want to implement their own confirmation strategy.

I'll work on the code and open a PR in the next few days so that we can discuss this further over something more concrete.