prolic / HumusAmqpModule

AMQP module for Zend Framework 2 to integrate RabbitMQ
https://humusamqp.readthedocs.io
MIT License
31 stars 13 forks source link

Consumer should return an object/value that can be checked as result #27

Closed thomasvargiu closed 8 years ago

thomasvargiu commented 9 years ago

Actually, consumer handle delivery like this:

    /**
     * @param AMQPEnvelope $message
     * @param AMQPQueue $queue
     * @return bool|null
     * @triggers delivery
     */
    public function handleDelivery(AMQPEnvelope $message, AMQPQueue $queue)
    {
        $params = compact('message', 'queue');
        $results = $this->getEventManager()->trigger('delivery', $this, $params);
        return $results->last();
    }

I think it's a problem. Anyone should be able to attach a listener at any point of the application, just for logging, debug, events. If a listener is attached after the real consumer process, the return value is not correct.

I suggest to create an object for the response and do something like this:

    public function handleDelivery(AMQPEnvelope $message, AMQPQueue $queue)
    {
        $params = compact('message', 'queue');
        $results = $this->getEventManager()->triggerUntil('delivery', $this, $params, function($v) {
            return ($v instanceof Object);
        });

        if ($results->stopped()) {
            return $results->last();
        }

        // no ack (?)...
    }

We should also allow users to specify listener priorities in the configuration, something like that:

return [
    'humus_amqp_module' => [
        'consumers' => [
            'my-consumer' => [
                'listeners' => [
                    ['My\\Listener' => 100]
                ]
            ],
        ],
    ],
];

I'm starting to implement this, I need to code and to test it.

prolic commented 9 years ago

I think what you want is .pre and .post events then.

    $params = compact('message', 'queue');
    $results = $this->getEventManager()->trigger('delivery', $this, $params);
    return $results->last();

is acutally the right code, but if you need post-listeners, let's extend it like this:

    $params = compact('message', 'queue');
    $results = $this->getEventManager()->trigger('delivery', $this, $params);
    $result = $results->last();
    $params['result'] = $result;
    $this->getEventManager()->trigger('delivery.post', $this, $params);
    return $result;

(little hacky, only for demonstration)

thomasvargiu commented 9 years ago

.pre and .post events are ok for some type of listeners. It should be the best idea, but we should always allow more than one listener for the delivery event.

Consider the follow situation:

This is useful for routing keys or something else based on the message.

The problem is that we can't consider a null return value because we should be able to identify a valid response, that's why an object. Then, the question is... What should we do when no listener handle the delivery event? throw a blocking exception?

Another option is to force the use of a single callback with an interface like the following, forcing to handle the message from a single point, just like before the events system, but just for the delivery event.

interface ConsumerCallbackInterface
{
    public function consume(Consumer $consumer, AMQPEnvelope $message, AMQPQueue $queue);
}

We should think about it. Let me know what you think.

prolic commented 9 years ago

If you want to decide how to handle a message (f.e. based on routing key), you should implement a strategy pattern in the consumer callback, instead of relying on attaching tons of events. Something like:

$handler = $this->getHandler($routingKey);
return $handler->handleConsume($consumer, $message, $queue);

But give me 1-2 days to think about it, I hope we can find a good solution.

thomasvargiu commented 9 years ago

You're right. The strategy pattern is a good solution. That's why I'm considering to allow just one handler for delivery event.

prolic commented 9 years ago

ping @olekhy, perhabs you have also some ideas?

prolic commented 8 years ago

The event system is removed