search

shaneharter / PHP-Daemon

Build production-ready Daemons using PHP 5.3+. Build fault-tolerant applications without the boilerplate.
Other
768 stars 166 forks source link

Fatal error: Call to a member function kill() on a non-object in Mediator.php on line 724 #41

Closed brunnels closed 10 years ago

brunnels commented 10 years ago

Keep getting this one when a worker dies and the whole daemon dies.

brunnels commented 10 years ago

Added this check to see if that resolves it.

if($this->process($call->pid) instanceof Core_Lib_Process) {
  $this->process($call->pid)->kill();
}
willebil commented 10 years ago

Can you clarify a bit more what happens in your case? I am doing some endurance tests with a basic setup of the daemon code, no special logic yet. But after a couple of tasks (between 200-250), a message starts popping up "recycling worker" and all processes are stopped. Looks like a similar issue, but I am not sure.

brunnels commented 10 years ago

What you're seeing is the built in worker auto restart functionality. $this->worker->auto_restart(false); in the daemon worker_setup will turn it off if you don't want it.

The issue I reported is happening when I have a fatal error that takes out the worker process. Then during the mediator timeout handler it tries to kill the worker process and then calls the defined timeout callback method in the daemon. The mediator throws a fatal error when it tries to call the kill() method on the non-existing worker process. That's why I added in the check to see if I get back a valid object from the process() method before trying to call kill() on it.

brunnels commented 10 years ago

I'm pretty sure in your case the auto_restart will eventually restart your workers but I think they have to hit their timeout so you have to wait at least that long. I could be wrong though.

shaneharter commented 10 years ago

That's exactly right but I have partially done code to implement a better solution to this. The way it should work -- had I realized this when I wrote it -- is simple. ProcessManager is notified when a child process exits (it listens for SIGCHLD). It just needs to notify the mediator and the mediator needs to take action.

I have a prototype of this in a local branch, i'll push it up and you can take a look.

On Thu, May 1, 2014 at 3:48 PM, Benjamin Runnels notifications@github.comwrote:

I'm pretty sure in your case the auto_restart will eventually restart your workers but I think they have to hit their timeout so you have to wait at least that long. I could be wrong though.

— Reply to this email directly or view it on GitHubhttps://github.com/shaneharter/PHP-Daemon/issues/41#issuecomment-41965867 .

brunnels commented 10 years ago

Is it the 2.1.1 branch? I'm looking at the commit and don't see any functionality changes other than removing the instructions so maybe you're still working on it? https://github.com/shaneharter/PHP-Daemon/commit/668c6bd5718beb9ab624b05e8976ea786e669988

willebil commented 10 years ago

Look closer, there is a subtile change in the mediator, and it looks like it is preventing the error from happening. Ran a test for >6 hours, and fully stable.

brunnels commented 10 years ago

Only thing added to mediator is unset($call); is there twice now which should do nothing because they're sequential with nothing between them.

shaneharter commented 10 years ago

Is it the 2.1.1 branch?

No, it's still local, it's something I hacked on a bit last September with another user, @soulhunter1987.

I'll dust it off and push what I have up to 2.1.1.

shaneharter commented 10 years ago

The Recycling Worker message is normal -- worker processes let themselves expire. The Mediator then should re-fork them. If that's not happening for you then that would go along with something @brunnels said -- that only the Aggressive forking srtategy works. In other words, the mediator isn't forking correctly for him in the non-aggressive strategy.

That would, for example, explain why after a while you start getting IPC errors. If you have no workers reading messages off the queue it won't work for very long. Kernel MQs aren't made for use-cases like that, that's the domain of Kestrel, Rabbit, Beanstalkd, et.

Similarly the other error -- the SHM error -- is the same story. Basically the way the IPC works is:

  1. Mediator writes the data to a shared memory address
  2. Mediator sends message to worker saying "Read the data from shm address X and run the job"
  3. Worker reads, runs, then writes the result back.
  4. Worker sends message to mediator saying "Return values available in shm address X"

Honestly as I mentioned elsewhere, the lack of resources and experience with these libraries in PHP hurts the success of this project. What I think would possibly be best is to remove the dependency on these libraries and instead just move over to a socket-based solution. That would take advantage of the queuing built in natively to sockets. Basically the worker opens a connection to the mediator. When run() gets called in the mediator it polls all the sockets for any pending messages. That isn't a good way to dispatch calls to a worker, though, because it would leave a worker idle until the daemon iterated over it and wrote a response. I need to think it over I suppose.

shaneharter commented 10 years ago

@brunnels The reaping code is VERY untested but I just pushed it up to 2.1.1a for you to take a glance at. I spent a few mins dusting it off and adding some code to streamline forking.

Basically we need to fork in 3 scenarios:

  1. When the mediator setup is run, when Aggressive is used
  2. The first call, when other strategies are used
  3. Whenever a process exits

Another use case which is currently supported (supposedly) is the Lazy strategy of forking on any call if there are no idle processes to handle the call. In other words, in the lazy strategy, if you put a max 3 workers, and called the worker 4 times in quick succession, the first 3 would lead to a fork() and the 4th would enqueue and wait. This idea of "fork only what we need as we need it" is attractive to me but it's not essential behavior and is totally optional.

brunnels commented 10 years ago

After this change that you just merged my PHP-Daemon implementation has been running for 3 days now. Before I couldn't keep it up for more than 12 hours or so.

I'll pull on Monday when I get into the office and do some testing with your new branch.

I would actually prefer a memcache solution to socket based simply because I remember something about php needing to be run with elevated permissions to open sockets. That would be a pain for me anytime I needed to update my production environment because of our required processes and controls.

Before I switched to PHP-Daemon I was using a file sync class for IPC to pass messages and data between processes using serialized arrays saved to a file. The file sync class implemented locks so 2 processes couldn't write to the array at the same time and you couldn't read from the array if another process was writing to it. So in the setters and getters for the arrays you just use while loops to check and wait for the lock. This worked out pretty well for me but I didn't have the process control working as well as your solution. I had intended to extend the sync class I was using to use memcache rather than files but hadn't gotten to it yet.

On Sat, May 3, 2014 at 7:07 PM, Shane Harter notifications@github.comwrote:

@brunnels https://github.com/brunnels The reaping code is VERY untested but I just pushed it up to 2.1.1a for you to take a glance at. I spent a few mins dusting it off and adding some code to streamline forking.

Basically we need to fork in 3 scenarios:

  1. When the mediator setup is run, when Aggressive is used
  2. The first call, when other strategies are used
  3. Whenever a process exits

Another use case which is currently supported (supposedly) is the Lazy strategy of forking on any call if there are no idle processes to handle the call. In other words, in the lazy strategy, if you put a max 3 workers, and called the worker 4 times in quick succession, the first 3 would lead to a fork() and the 4th would enqueue and wait. This idea of "fork only what we need as we need it" is attractive to me but it's not essential behavior and is totally optional.

— Reply to this email directly or view it on GitHubhttps://github.com/shaneharter/PHP-Daemon/issues/41#issuecomment-42119977 .

ekzobrain commented 10 years ago

What OS do you task about? I never got into situation when PHP needed extra permisions to open sockets... There may be problems with tcp/udp sockets, but unix sockets should always function out of the box. This functions is best suited for it: http://php.net/manual/en/function.socket-create-pair.php

brunnels commented 10 years ago

I'm not certain that's the case and I could certainly be wrong as I haven't tested it but I've got that memory of pain somewhere in the deep recesses of my brain popping up and it had something to do with sockets and permissions but I don't remember the specifics. Production is RHEL5 soon to upgrade to RHEL6 and development is the latest ubuntu server.

On Sun, May 4, 2014 at 6:32 AM, Dmitry notifications@github.com wrote:

What OS do you task about? I never got into situation when PHP needed extra permisions to open sockets... There may be problems with tcp/udp sockets, but unix sockets should always function out of the box. This functions is best suited for it: http://php.net/manual/en/function.socket-create-pair.php

— Reply to this email directly or view it on GitHubhttps://github.com/shaneharter/PHP-Daemon/issues/41#issuecomment-42130789 .