register function only once to gearman worker

[cturbelin]

Fix related to issue #1140 to only register the worker function only once to gearman

[makasim]

The message property looks suspicion to me. As well as public function receive_message which pollutes class interface.

What about this solution:

<?php

$message = null;

if (!$this->registered) {
    $this->worker->addFunction($this->destination->getName(), function (\GearmanJob $job) use (&$message) {
        $message = GearmanMessage::jsonUnserialize($job->workload());
    });
    $this->registered = true
}
            

[cturbelin]

Thanks for your comment and sorry for the delay !

I tried this solution, but it's not working (no message is handled after the first loop)
I suspect it's because the reference to $message is not valid anymore on the loops after the first one (the scope of this local variable on the next calls after passing it to the anonymous function is not clear to me).

The use of static variable is working, but need the second assignment (the fist one is only run when the static variable is created if I'm not wrong).

 static $message = null;
 $message = null;

It's working but I'm not confortable with this solution.
Other solution would be to use an extra class (possibly anonymous) to handle the message, it could be cleaner and safer than the use of a static variable, no ?

The block can become

            if (!$this->handler) {
                
                $this->handler = new class {
                    
                  protected $message;
                  
                  public function handle(\GearmanJob $job) {
                      $this->message = GearmanMessage::jsonUnserialize($job->workload());
                  }
                  
                  public function getMessage() {
                      return $this->message;
                  }
                  
                  public function reset() {
                      $this->message = null;
                  }
                };
                
                $this->worker->addFunction($this->destination->getName(), [$this->handler, 'handle' ]);
            } else {
                $this->handler->reset();
            }

It's working (and just a draft comments are missing), but a non anonymous class would be better, no (I dont know your policy about it) ?
I can rewrite and redo a PR with the solution you find the most acceptable !

[makasim]

let's keep the current one, it simple and serve the same purpose

What about moving addFunction logic to the __consturct method? If that possible we can get rid of registered property. If not, that's fine as it is.

[cturbelin]

Yes, but the current implementation register the function to gearman at each loop. To add it in the constructor we will need a reference to a persistent variable for message content (so it cannot be a local variable created in constructor) so either static or embedded into a class as it should be accessed by the receive() function later, or I am missing something (possible anyway).

[makasim]

I am fine with message property. Looks like it is unavoidable.

[cturbelin]

Yes, if the worker callback is only registrer once we need either a property or a class to embed it
so we can just put this in constructor (using the message prop):

$this->worker->addFunction($this->destination->getName(), function (\GearmanJob $job) {
            $this->message = GearmanMessage::jsonUnserialize($job->workload());
});

and receive() becomes :

 //  [...]  timeout & handler
   $this->message = null;
    try {
        $this->worker->work();
    } finally {
        restore_error_handler();
    }
   return $this->message;
}

It's working on my side, I could make the PR if it's ok !

[makasim]

Yeap, that looks great. Thank you for your effort.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.