gearman_client_set_server_option calling failed

[lkebin]

Hi,

I'm using wcgallego/pecl-gearman extension for my php project.

I have two gearman job servers A and B. If one of job servers was down but the other was up, GearmanClient::doBackground will be failed.

I got exception: Failed to set exception option from wcgallego/pecl-gearman extension.

Check wcgallego/pecl-gearman source code
https://github.com/wcgallego/pecl-gearman/blob/7480e92c630db1e5f1a51633fcb80ffdda4bd3eb/php_gearman_client.c#L290-L314

The reason that gearman_client_set_server_option calling failed.

I found the similar issue report from launchpad:
https://bugs.launchpad.net/gearmand/+bug/1098413

Is that fixed ? or we don't need call gearman_client_set_server_option ?

Thanks.

[p-alik]

Fix Released

[lkebin]

Hi @p-alik

I also found the following description from gearman.info:

gearman_client_set_exception_fn() will only be called if exceptions are enabled on the server. You can do this by calling gearman_client_set_server_option().

An example of this:

const char *EXCEPTIONS="exceptions";
gearman_client_set_server_option(client, EXCEPTIONS, strlen(EXCEPTIONS));

Just for confirm, is it correct for latest gearman version ?

[p-alik]

Just for confirm, is it correct for latest gearman version ?

as far as I know it's correct.

see also http://gearman.org/protocol/

OPTION_REQ

    A client issues this to set an option for the connection in the
    job server. Returns a OPTION_RES packet on success, or an ERROR
    packet on failure.

    Arguments:
    - Name of the option to set. Possibilities are:
      * "exceptions" - Forward WORK_EXCEPTION packets to the client.

[lkebin]

Ok

gearman_client_set_server_option will call connnection_loop function, and connection_loop will be failure when any connection break. will occur the failover function does not work.

https://github.com/gearman/gearmand/blob/master/libgearman/universal.cc#L554-L612

Can we skip broken connection in connection_loop function ?

[p-alik]

Can we skip broken connection in connection_loop function ?

I'm afraid it want help because regarding to your initial description it seems to be an issue of pecl-gearman implementation.

The change could lead to misbehavior of gearmand in general because connection_loop is also involved in:

• gearman_return_t gearman_echo
• gearman_return_t cancel_job

[lkebin]

Hi @p-alik ,

Could you have some suggestions about gearman_client_set_server_option function call ?

Should we call it follow the addServer[s] ? or other correct place ?

If project team can provide some use case about gearman_client_set_server_option function, will be great.

Thanks.

[jeff-minard-ck]

See wcgallego/pecl-gearman#59 where this was originally brought up.

The PECL extension has, for quite some time, had the php routine addServer()/addServers() make an additional call to set the exception intent. It seems that in the core library, the call to gearman_client_set_server_option at some point went from a lazy loaded option to an at-call-time setting of the server side option. This change in behavior causes a connection to start when the php code for addServer() is called, despite the php method's claim that it would simply "add servers to a list, not connect".

More problematically, since the PECL code would detect a failure in setting the server side option and throw an exception, the addServer() php call now generates exceptions when addServer()ing instances which are down. This is not the intended functionality.

How should the PECL extension correctly handle the change to gearman_client_set_server_option so that the php routine for addServer() can correctly set a list of up/down servers and also still set the exception handling?

Ideally, there's a way to go back to the lazy-loading approach as making connections on every addServer() is not ideal.

I hope this helps to clarify what's going on and that I've got it all straight :D

[p-alik]

@lkebin

Could you have some suggestions about gearman_client_set_server_option function call ?

Unfortunately no, because I use only OPTION_REQ request for the purpose.

Should we call it follow the addServer[s] ? or other correct place ?

Why gearman_client_set_server_option is called within PHP_FUNCTION(gearman_client_add_servers) block?

[p-alik]

@jeff-minard-ck,

it seems that in the core library, the call to gearman_client_set_server_option at some point went from a lazy loaded option to an at-call-time setting of the server side option

if your assumption is that late changes on gearmand implementation lead to the PECL's addServer-issue. We could easily reproduce it because there are only tree commits on
there are only three commits on libgearman/client.cc while project is on github.

1. 19684aa
2. 3e04a5e
3. 5d5cf65

[jeff-minard-ck]

The previous version of the PECL plugin was based on libgearman 1.1.8, which I can't find a direct tag/source for. However, I pulled up the 1.1.9 branch (https://github.com/gearman/gearmand/blame/1.1.9/libgearman/client.cc#L1802) which has the code causing the connection:

gearman_success(gearman_server_option(client->universal, option))

If you click the View blame prior to this change on the function definition line, we end up here

This function is vastly different:

bool gearman_client_set_server_option(gearman_client_st *self, const char *option_arg, size_t option_arg_size)
{
  if (self)
  {
    gearman_string_t option= { option_arg, option_arg_size };
    return gearman_request_option(self->impl()->universal, option);
  }

  return false;
}

And looks like it's setting an option to be used later, rather than trying to connect and add the option upon calling gearman_client_set_server_option()

[p-alik]

The previous version of the PECL plugin was based on libgearman 1.1.8, which I can't find a direct tag/source for.

Version 1.1.8 released at 2013-06-07 still exists on launchpad
https://launchpad.net/gearmand/1.2/1.1.8

Indeed implementation of gearman_client_set_server_option has been changed meanwhile. From my point of view PECL should adopt changed behavior of gearman_client_set_server_option but the maintainer should decide what should be done.

[jeff-minard-ck]

@p-alik While I could argue that such a change is BC break, it's been this way for a while and other implementations have clearly adapted. I'm, personally, fine with the PHP extension changing, as posted earlier:

How should the PECL extension correctly handle the change to gearman_client_set_server_option so that the php routine for addServer() can correctly set a list of up/down servers and also still set the exception handling?

I was hoping there are some examples of other implementations that handled this API change in so much that the PHP author (@wcgallego) might be able to lean on.

[wcgallego]

reading through this thread.

quick recap: I took over maintaining around PHP 7 transition ~3 years ago. The project hadn't seen much in the way of progress for a while before that, hence falling behind.

If the gearman core lib has has this change for quite some time, then yeah it does seem on the part of the pecl code to get in line.

I'll look around for how others are adapting, but if anyone has a pointer for another language doing something similr, lmk!

[SpamapS]

1750815

That's where it changed to connecting. It's possible that wasn't meant to happen in a patch release such as 1.1.9 and we should have made it 1.2 at that point. Brian left the code tree fallow for a while and we just didn't notice this change when we resumed releases. I'm very sorry that this has caused so much confusion.

It is a BC break, but the original libgearman policy was that 1.1 was not a stable interface yet. So, I'm sorry again that it caused you issues, but at this point, I think we're just going to have to go forward with things the way they are. I'm open to thoughts about reverting it also. This might be better as a mailing list discussion.

[wcgallego]

Thanks @SpamapS for the input. I'm hoping to work around it in the covering pecl lib here, but trying to sort through what the expected case should be then. It's still going to throw a warning even without setting the option.