Add graceful max execution timeout.

[d-ph]

Allows for running consumer up to certain time, which guarantees, that resources are cleaned up periodically and unobtrusively (assuming, that workers are then restarted by something like supervisord).

[skafandri]

php -d max_execution_time=1800 rabbitmq:consumer ... wouldn't achieve the same result?

[d-ph]

Using set_time_limit() and/or max_execution_time would interrupt tasks in progress. My solution gracefully waits for any currently running task to finish. I wouldn't like my workers to be "killed" mid-task, because of timeout.

Moreover, for some reason invoking consumer with -d max_execution_time=1800 or manually changing the timeout with set_time_limit() didn't seem to work at all in my case (despite the fact, that I removed the set_time_limit() instruction in bin/console).

But most of all: I really value the fact, that tasks wouldn't be interrupted, even if the timeout hits during tasks' progress. And I don't think I'm the only one thinking this way.

Thanks.

[stloyd]

I would rename those two options into something like:

consumers:
    upload_picture:
        graceful_max_execution:
            timeout: 1800 # 30 minutes
            exit_code: 10 # by default: 0

[stloyd]

No need to call method, just call variable directly.

[stloyd]

assertSame() should be used.

[stloyd]

This if could be opposite & the duplicated return with self::TIMEOUT_TYPE_IDLE could be removed then.

[d-ph]

I'm sorry, but I disagree.

The code is not duplicated. This return statement is the same as the one a the end of the method, but the intention is different. The code after line 294 means: "do this, if Graceful Max Execution feature is not enabled". The code in the if statement starting on line 280 means: "Use Idle Timeout, if it's less than the computed remaining allowed execution". Currently, they are same in terms of code. But if tomorrow someone wanted to add some other crazy type of timeout, then they would modify the code after line 294. I bet they wouldn't think: "Ok, that other guy from Graceful whatever relied on the code after line 294 to return the Idle timeout". And then my code breaks.

Clear naming and no duplication are my prime principles. However I wouldn't trade "being proper" for "being pragmatic" here.

Please confirm, that you'd like me to remove the "duplication" despite what I said.

[stloyd]

Type-hint is required here.

[stloyd]

You should add some validation to be sure you get seconds, maybe on configuration level?

[d-ph]

You mean validation, that I get an integer? Or that I get seconds and not minutes or hours?

[d-ph]

I added validation, that the value is integer on configuration level. Please let me know if this is enough.

[d-ph]

@stloyd Hi, I replied to your some of your code review comments. I'm not sure, whether you got a notification from github about that fact, so I thought I'd at-mention you as well. Thanks.

[stloyd]

@d-ph Sorry it took so long & thanks for your work!

[acasademont]

HI @d-ph for what I can see this doesn't apply to multiple consumers, right?

[d-ph]

@acasademont

Yeah, you're right, it's per consumer thing, which I guess is good and bad. Good, because it allows you to enable this feature per consumer basis (although I'm not sure why you wouldn't want to use my feature for all of the consumers). Bad, because it requires you to mention the same config for many (all?) consumers. What I personally do is I utilise YAML's hashmap merge feature to cut down on duplication like this:

  (...)

  producers:
    default:
      connection:       default
      service_alias:    default_rabbitmq_producer

  consumers:
    consumer1: &consumer_template
      connection: default
      queue_options: { name: 'consumer1' }
      callback: 'my_app_bundle.queue.consumer.consumer1'
      qos_options: { prefetch_size: 0, prefetch_count: 1, global: false }
      graceful_max_execution:
        timeout: '%rabbitmq.consumers_graceful_max_execution_timeout%'
        exit_code: '%rabbitmq.consumers_graceful_max_execution_timeout_exit_code%'

    consumer2:
      <<: *consumer_template
      queue_options: { name: 'consumer2' }
      callback: 'my_app_bundle.queue.consumer.consumer2'

    consumer3:
      <<: *consumer_template
      queue_options: { name: 'consumer3' }
      callback: 'my_app_bundle.queue.consumer.consumer3'

I highly recommend following this pattern.

Hope this is what you asked about. Any questions - shoot.

@d-ph

[acasademont]

Wow, I wasn't aware of that yaml feature, thanks @d-ph, it will come handy for our consumers :p
Although my question wasn't exactly that one, I was talking about the "multiple_consumers:" That you can declare in the bundle and that can read multiple queues in the same PHP process. As the MultipleConsumer class extends from the Consumer one it should be easy to add support for the graceful_max_execution option for those too.

[d-ph]

I wasn't aware of the multiple_consumers: feature. I'll take a look this weekend to read up on it and see how difficult it'd be to support it. But yeah, from what you said I reckon it should be fairly easy to expose the graceful max exec feature for those consumers as well.

---

I just realised, that there's an open PR with this addition already: #468