Page MenuHomePhabricator

Domain Events: allow listeners to execute code via the job queue
Open, Needs TriagePublic

Description

Event listeners should have a simple way to invoke code in a job, without the need to implement and register a dedicated job class.

Implementation idea: if we register subscriber objects by name, we can define a generic job class that is constructed around the name of the subscriber and the name of a method to call on that subscriber. When executed, the job will ask the event dispatcher for the subscriber by name, then invoke the method. With the appropriate support in EventSubscriberBase, this will allow listeners to schedule any method on the subscriber for execution via a job:

MySubscriber extends EventSubscriberBase {
    public function handleFooEvent ( FooEvent $event ) {
        if ( !$event->isInteresting() ) {
            return;
        }

        $this->schduleCall( 'doSlowStuffWithFooEvent', $event );
    }


    public function doSlowStuffWithFooEvent( FooEvent $event ) {
        // slow stuff
    }   
}

Note that this supercededs T384609: Domain Events: support listener invocation via the job queue: Executing the listener itself via the job queue is ineffecient and provides too little control. We'd schedule at least one job per event that has subscribers using the in-job invocation mode, but probably more: using one job per listener is better for retry behavior and if we need to re-route specific jobs/handlers to a separate stream or even a dedicated cluster. This seems inefficient, since current usage patterns in hook handlers show that the actual business logic will only be execute in a small subset of cases - for most events, the handler exists early. So it would be more sensible to only shedule a job if we already know that the business logic should actually run.

Event Timeline

Change #1118804 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] WIP: DomainEvents: add convenience for scheduling jobs

https://gerrit.wikimedia.org/r/1118804

I'll move my comment from T384609 so we can have the discussion here.


I was asking some questions about WIP: DomainEvents: add convenience for scheduling jobs (1118804) in T379935#10553701. I asked them there because they were related to how we might implement cross-wiki event receiving. I want to ask some more questions about this job implementation, and now the details are more relevant to this task. Moving that convo here.


I asked why DomainEvents needs to have a specific DomainEventsJob abstraction, when it seems what that is doing is just serializing a callback to fire later in a job. I'm wondering if instead, this could be a Job that is totally unrelated to DomainEvents, and then DomainEvents could use it to invoke subscribers in a job later.

@daniel wrote

the goal to to make it easier for subscribers to schedule jobs, by taking advantage of the infrastructure already in place for subscribers. We can't do this outside subscribers, because then we have no way to refer to object instances by name

Instance by name, or class by name? Do you need instances by name? I think what you need are a class to instantiate and the parameters to use to instantiate it.

Well... which class should it instantiate? The subscriber sclass?

Could this be serialized in the job itself? IIUC, ObjectFactory can do this. E.g.

class ObjectFactoryCallbackJob extends Job {
// ...
    public static function newSpec(
                string $jobType,
		string $objectFactorySpec,
		string $callbackMethodName,
		array $callbackMethodParams,
	): JobSpecification {

		return new JobSpecification(
			$jobType,
			[
				'objectFactorySpec' => $objectFactorySpec,
				'callbackMethodName' => $callbackMethodName,
				'callbackMethodParams' => $callbackMethodParams
			]
		);
	}

    public function run() {
                $instance = ObjectFactory::getObjectFromSpec( $params['objectFactorySpec'] );
                $methodName = $params['callbackMethodName'];
                return call_user_func_array( [$instance, $methodName], $params['callbackMethodArgs'] );
	}

}

Or some variation on that.

Could you use something like that to build DomainEvent job invocation? This would be more generic, and avoid having to build the Job-ness directly into the DomainEvent system. You wouldn't need to add stuff into EventDispatcherBase to handle getting subscribers and invoking subscribers by name in a job. EventDispatcherBase doesn't need to know that the subscriber is using a job to execute something later.

EventSubscriberBase could have some nice sugar to schedule a job like this:

function invokeLater($method, $event) {
   // submit ObjectFactoryCallbackJob 
}

and/or this could be a invocation mode and done by EventDispatcherBase?

// e.g.  dispatchAs method
if ($this->invocationMode == 'job') {
   // submit ObjectFactoryCallbackJob with appropriate subscriber method as callback
}

Thoughts? I'm sure I'm missing something that would make this impractical or difficult.

@Ottomata wrote

I asked why DomainEvents needs to have a specific DomainEventsJob abstraction, when it seems what that is doing is just serializing a callback to fire later in a job. I'm wondering if instead, this could be a Job that is totally unrelated to DomainEvents, and then DomainEvents could use it to invoke subscribers in a job later.

Yes, there is no strong conceptual relationship to domain events. However, it is convenient for these jobs to make use of the fact that subscriber objects are already registered in the DomaineventDispatcher. That way, they don't have to be registered and instantiated twice, once for handling events, and once for handling job callbacks.

the goal to to make it easier for subscribers to schedule jobs, by taking advantage of the infrastructure already in place for subscribers. We can't do this outside subscribers, because then we have no way to refer to object instances by name

Instance by name, or class by name? Do you need instances by name? I think what you need are a class to instantiate and the parameters to use to instantiate it.

Yes, the object spec - and we already have that in the event dispatcher.

We could inject the object spec into the job, instead of the instance name. That would be totally generic, the job wouldn't need to know anything about the event dispatcher. But it's tricky because instances of EventSubscriberBase expect to be initialized withing the context of a domain event dispatcher...

Basically, it's more convenient to implement it based on the event dispatcher. But that fact is an implementation detail… The job class should be internal, and we could change the mechanism in the future.

Hm, I suppose I see what you are saying. And I guess you are reducing the DomainEventSource interface in your patch (removing registerSubscriber), which I like.

Basically, it's more convenient to implement it based on the event dispatcher.

You do have to add a bunch of Job specific stuff to EventDispatcherBase though. Whereas if it truly was "convenience for listeners to submit jobs", building that sugar on a more generic DeferredUpdateJob (or whatever) would make the Dispatcher system totally unaware of Job related stuff (as you say), which seems...better to me somehow? Yes how the job is scheduled is an implementation detail, but wouldn't it be nice if that detail was not inside of EventDispatcherBase at all?

it's tricky because instances of EventSubscriberBase expect to be initialized withing the context of a domain event dispatcher...

I suppose...this is because the listener will expect to have whatever service objects are injected into the Subscriber available?

I'd expect a DeferredUpdateJob with Subscriber sugar could just have these service objects injected well, but I suppose it would be difficult to it in exactly the same way they are injected into Subscriber?


Anyway, okay I'll drop it. Something seems a bit muddled (exposing EventDispatcherBase internals to DomainEventJob), but I suppose it isn't that bad, and I admit I don't understand all the complexities involved.

You do have to add a bunch of Job specific stuff to EventDispatcherBase though. Whereas if it truly was "convenience for listeners to submit jobs", building that sugar on a more generic DeferredUpdateJob (or whatever) would make the Dispatcher system totally unaware of Job related stuff (as you say), which seems...better to me somehow? Yes how the job is scheduled is an implementation detail, but wouldn't it be nice if that detail was not inside of EventDispatcherBase at all?

There's no EventDispatcherBase - do you mean EventDispatchEngine, or EventSubscriberBase? EventSubscriberBase has to know about it, because that's what extension code builds on. But putting some of the logic into EventDispatchEngine is an implementation detail. The only people who should know or care about this are the ones working on the implementation of EventDispatchEngine.

It could be avoided by introducing another class, an EventSubscriberRegistery, that gets composed into the EventDispatchEngine . Doable, but seems a lot of hassle for litte gain.

I'd expect a DeferredUpdateJob with Subscriber sugar could just have these service objects injected well, but I suppose it would be difficult to it in exactly the same way they are injected into Subscriber?

It would be possible, but to comes down to duplicating code and object structures, for no particularly good reason.

Anyway, okay I'll drop it. Something seems a bit muddled (exposing EventDispatcherBase internals to DomainEventJob), but I suppose it isn't that bad, and I admit I don't understand all the complexities involved.

I am thinking of the implementations as "friend classes". They know each other's details to achieve a goal in an efficient and convenient way. They hide all these details from callers.

I agree that this could be cleaner. But perfectly clean is often unusable and unmaintainable. Learned that the hard way. So I try to keep the interfaces clean, and allow myself some fudging in the implementations.

There's no EventDispatcherBase - do you mean EventDispatchEngine, or EventSubscriberBase?

I'm sorry oops. Yes I meant EventDispatchEngine. I meant the implementation of DomainEventDispatcher (and DomainEventSource)

Thought: if we do this, is there a way for the subscriber to submit an event to be processed by a foreign wiki?

It would probably be nice to support this!