php / pecl-networking-gearman

PHP wrapper to libgearman
https://pecl.php.net/package/gearman
Other
33 stars 25 forks source link

What about publishing a new version on pecl? #12

Closed mlocati closed 7 months ago

mlocati commented 2 years ago

The master branch contains the fix https://github.com/php/pecl-networking-gearman/commit/7da13e4babc17067b2b45d6b37041c3c8ed91637

Without that patch, we can't use this extension on PHP 8.1, since we have this issue:

Warning: PHP Startup: Unable to load dynamic library 'gearman' [...]
Error relocating [...]/gearman.so: ZVAL_NEW_ARR: symbol not found
oleg-st commented 2 years ago

@rlerdorf could you release new version with php 8.1 support?

mikevrind commented 2 years ago

A new version would be highly appreciated.

Until a new release has been made, you could always build based on the master branch :)

MyDigitalLife commented 2 years ago

+1 for a new release. As php 7.4 will no longer be supported in a few months a php 8.0 supported version is needed. If there are no plans to support this the project schould be marked as abandoned.

esabol commented 2 years ago

Who decides when (or where) to release? I'm not really clear on who has maintainer status on this project. It's under the PHP organization's GitHub?

trenshaw commented 2 years ago

I can confirm the 'master' branch currently passes 'make test' and installs against libgearman v1.1.19 and PHP 8.1.2 (the default packages in Ubuntu 22.04). Perhaps 'master' could now be tagged a release candidate for 2.1.1? There will be many other Ubuntu 22.04 users facing the same issue with 2.1.0.

zacek commented 2 years ago

+1: another reason for the new release: there is a memory leak fix in the master branch.

esabol commented 2 years ago

@zacek wrote:

there is a memory leak fix in the master branch.

Could issue #17 be related to that?

zacek commented 2 years ago

@esabol I don't think so. My fix was memory leak in the client. The issue in #17 is in the worker. From the report it is not clear what the worker was doing but generally, worker creates response object (allocating memory) and client is responsible for freeing it. So, my fix was correct, IMHO.

What I find very suspicious in the worker code which could cause the problem reported in #17 are these lines in the worker constructor:

    worker = Z_GEARMAN_WORKER_P(return_value);

    if (gearman_worker_create(&(worker->worker)) == NULL) {
        zval_dtor(return_value);
        GEARMAN_EXCEPTION("Memory allocation failure", 0);
    }

    worker->flags |= GEARMAN_WORKER_OBJ_CREATED;

Why is zval_dtor called for the return_value if the allocation fails? If you look into the client constructor, which does almost the same thing, the destructor for the return_value is not called at all.

IMHO, the scenario for #17 is: memory allocation for the worker fails and destructor is called on null. This also hides the real exception - insufficient memory - which would be thrown afterwards. This could easily be checked in two ways.

  1. the easy way: increase the memory and check if it mitigates the problem
  2. the more difficult way: rebuild the gearman pecl extension without the zval_dtor line in the worker constructor and try again. If you get memory allocation problem it is this problem and the destructor line should be removed
esabol commented 2 years ago

Thanks for taking a look at that, @zacek. Unfortunately, my workers aren't implemented in PHP. I hope someone can test your prospective changes.