Webador / SlmQueue

Laminas / Mezzio module that integrates with various queue management systems.
Other
137 stars 56 forks source link

create new event for trigger as current one is marked with stopPropagati... #137

Closed basz closed 8 years ago

basz commented 10 years ago

...onWorkerEvent::EVENT_BOOTSTRAP is trigger but since $e->stopPropagation is called the event never reaches any listeners. Create a new event with same payload events now go to listeners.

bakura10 commented 10 years ago

Why are you stopping propagation then?

basz commented 10 years ago

Hmm, did't realize what i am doing here... I don't know why BOOTSTRAP is stopped there. The event must be 'unattached' for sure.

The task of the AttachQueueListenersStrategy is to attach other strategies. Including ones that attach to BOOTSTRAP Currently these strategies don't get them...

@juriansluiman remember why the event is stopped from propagation at that point?

basz commented 10 years ago

This needs to be fixed.

We must retrigger a BOOTSTRAP event again as the time of the original call to trigger BOOTSTRAP no other listeners are attached (as attaching is the responsibility of the AttachQueueListenersStrategy).

Either we don't stopPropagation and retriever the event as is or we create a new event. stopPropagation is irrelevant in that case anyway.

IMO it would be simplest to not stop propagation and retrigger

juriansluiman commented 10 years ago

The idea was this:

  1. Bootstrap is called to pre set-up the various things you need
  2. On bootstrap, a listener is called which can attach new strategies based on the queue that is used
  3. Some specific queue strategies might use bootstrap as well

The problem is, on bootstrap you can add new listeners for bootstrap, but they will never be triggered. So the idea was to detach the AttachQueueListenersStrategy, then stop the propagation of the event and then trigger the same event again, so all (new) strategies could be running bootstrap as well.

It is not the nicest thing, but I am not sure how we can solve this otherwise without introducing yet another event (which makes the process only more complex).

basz commented 10 years ago

Agreed, but;

Definitely -1 for another event, so

Either we stopPropagation (to stop the current event flow) and we are obligated to trigger a new event. We can not trigger the same event if stopPropagation has been called on it as then it will not propagate in the new trigger cycle (. (my first attempt)

Or we trigger the same event again without calling stopPropagation on it. The AttachQueueListenersStrategy is the (we can be sort of sure of that) the only Listener that is attached at that time. (my second attempt)

So i'm going back and forth between stopPropagation + new event and don't stop and same event. Now I favor explicitly stopping propagation and retriever a new event as that is more in 'the line of thought'.

final thoughts? @bakura10

(5 minutes later) damn... now I am confused, i did not know about this : https://github.com/zendframework/zf2/blob/master/library/Zend/EventManager/EventManager.php#L205

let me re-investigate and get back. something is wrong...

basz commented 10 years ago

The problem is related to the way attaching is done...

Currently this happens in two steps.

If one of these listens to BOOTSTRAP it will receive that event twice. This happens because the propagation flag is reset every time EventManager::trigger is called.

If one of these listens to BOOTSTRAP it will receive that event once as expected.


I propose a simplification; Let AttachQueueListenersStrategy handle everything.

  1. In Workerfactory::createService we now only attach AttachQueueListenersStrategy hardcoded.
    • When the worker triggers BOOTSTRAP we now know what is listening, just the AttachQueueListenersStrategy.
  2. Within AttachQueueListenersStrategy we detach AttachQueueListenersStrategy and attach all other strategies and retrigger as we currently do. Thus not only the 'queue' strategies.
    • When we do this we are able to correctly merge the configurations of both the worker configurations and the queue configurations. This ensures a strategy cannot be instantiated and attached more then once - which may happen currently.
    • Overriding strategy configuration now becomes very simple and far less confusing. (You might have strict memory requirements in general, but need a large setting for one particular queue. This is currently not possible, as two instances of the strategies are attached, both with there own configuration).
    • Opportunity to have only one place where configuration is normalized (numeric key based vs string key based with options, existence of priority, etc.).
    • Opportunity to have only one place where strategies are attached.
    • AttachQueueListenersStrategy should be renamed to AttachListenersStrategy or possibly BootstrapStrategy and it should be removed from the configuration altogether, it becomes hardcoded.

See here for a quick peek of the proposed changes, but I am more interested in your general thoughts atm (am available on irc) https://gist.github.com/basz/2e86c73de71392787e61 thanks

basz commented 10 years ago

ping @bakura10 @juriansluiman

bakura10 commented 10 years ago

I honestly have no opinion on that. I've always find this part of new SlmQueue very very confusing... So I'm in favor of everything that could simplify it...

Overriding strategy configuration now becomes very simple and far less confusing. (You might have strict memory requirements in general, but need a large setting for one particular queue. This is currently not possible, as two instances of the strategies are attached, both with there own configuration).

I'm not sure to understand that. You are saying that you can't have one queue with different settings than the other? Isn't this simply because the plugin manager here is configured as sharing instances by default?

basz commented 10 years ago

nope not shared https://github.com/juriansluiman/SlmQueue/blob/master/src/SlmQueue/Strategy/StrategyPluginManager.php#L16 https://github.com/juriansluiman/SlmQueue/blob/master/src/SlmQueue/Strategy/StrategyPluginManager.php#L16

This confusion comes from the fact we have strategies attached to all workers in the first pass. from the slim_queue.worker_strategies.default config And then later on (in the AttachQueueListenersStrategy) attach more listeners for the queue we are working on from slim_queue.worker_strategies.queues.queueName config.

Merging of one with the other is not really possible ATM. Set a strategy in slim_queue.worker_strategies.default and you can not override it in slim_queue.worker_strategies.queues.queueName

This is only possible for invokables. That does not work with Factories (the options are then for the factory, which is instantiated only once), which IMO is a flaw in the SM design.

$pluginManager->get(’service’, $options); 

This would probably work

$strategy = $pluginManager->get(’service’);
$strategy->setOptions($options);

On 27 okt. 2014, at 22:01, Michaël Gallego notifications@github.com wrote:

I honestly have no opinion on that. I've always find this part of new SlmQueue very very confusing... So I'm in favor of everything that could simplify it...

Overriding strategy configuration now becomes very simple and far less confusing. (You might have strict memory requirements in general, but need a large setting for one particular queue. This is currently not possible, as two instances of the strategies are attached, both with there own configuration).

I'm not sure to understand that. You are saying that you can't have one queue with different settings than the other? Isn't this simply because the plugin manager here https://github.com/juriansluiman/SlmQueue/blob/master/src/SlmQueue/Strategy/AttachQueueListenersStrategy.php#L79 is configured as sharing instances by default?

— Reply to this email directly or view it on GitHub https://github.com/juriansluiman/SlmQueue/pull/137#issuecomment-60668571.

bakura10 commented 10 years ago

You can set options in factory in SM if the factory implements MutableCreationOptions (https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/MutableCreationOptionsInterface.php).

This is a design flaw in the SM, as you said.

basz commented 10 years ago

wauw - totally missed that

On 27 okt. 2014, at 22:29, Michaël Gallego notifications@github.com wrote:

You can set options in factory in SM if the factory implements MutableCreationOptions (https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/MutableCreationOptionsInterface.php https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/MutableCreationOptionsInterface.php).

This is a design flaw in the SM, as you said.

— Reply to this email directly or view it on GitHub https://github.com/juriansluiman/SlmQueue/pull/137#issuecomment-60672710.

bakura10 commented 10 years ago

I'm the culprit of this ignominy.