[Messenger] Add a memory limit option for `ConsumeMessagesCommand` #26975

sdelicata · 2018-04-18T13:44:49Z

Q	A
Branch?	master for features
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

New feature to add a memory limit option to the ConsumeMessagesCommand.

bin/console messenger:consume-messages <receiver-name> -m 128

linaori · 2018-04-18T14:01:26Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

+
+Use the --memory option to limit the memory consumed by the worker:
+
+    <info>php %command.full_name% <receiver-name> --memory=128</info>


Would personally call it --memory-limit to be the same as in php. It's also not exactly clear that it's in M. Would it be an idea to use G, M, K support here, just like with php?

Good idea, I did it

sroze · 2018-04-18T15:01:19Z

Same as for the other PR, could you have some tests? :)

sroze · 2018-04-18T15:04:05Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

@@ -61,6 +63,10 @@ protected function configure()
 Use the --limit option to limit the number of messages received:

    <info>php %command.full_name% <receiver-name> --limit=10</info>
+
+Use the --memory-limit option to limit the memory consumed by the worker. Use PHP shorthand byte values [K, M or G]:


- Use the --memory-limit option to limit the memory consumed by the worker. Use PHP shorthand byte values [K, M or G]: + Use the --memory-limit option to stop the worker if it exceeds a given memory usage limit. You can use shorthand byte values [K, M or G]:

sroze · 2018-04-18T15:06:20Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+
+    private function convertToOctets(string $size): int
+    {
+        if (\preg_match('/^(\d+)(.)$/', $size, $matches)) {


I'd be stricter than .. Maybe just (G|M|K) and throw an exception if nothing is recognized to prevent any issue.

yceruto · 2018-04-19T19:15:45Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+            throw new \InvalidArgumentException('Invalid memory limit given.');
+        }
+
+        return (int) $size;


Could (int) casting be removed? seems unnecessary after \d+ and ... * 1024 ....

True, unnecessary anymore, thx

yceruto · 2018-04-19T19:31:53Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+
+    public function stop(): void
+    {
+        $this->decoratedReceiver->stop();


I wondering if should we throw some kind of exception or log after that? I mean, right now it's stopped silently, how can we know if it was finished by memory limit or because it has received all messages?

@yceruto Good idea.
@sroze Did you already imagined such a notification mechanism?

I don't see any reason throwing an exception for this. What is your use-case? Do you need to know this for something?

Actually, it is probably a good idea to log such behavior (no exception). Maybe an optional LoggerInterface argument that would be called with info when that happens? (note that it wouldn't be on this line but when you actually detect the memory usage line 41 of this line).

@lyrixx any PoV on the logging level?

I disagree exception too, but the optional LoggerInterface is a good idea

yceruto · 2018-04-19T19:32:35Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+    {
+        $this->decoratedReceiver = $decoratedReceiver;
+        $this->memoryLimit = $this->convertToOctets($memoryLimit);
+        $this->memoryResolver = $memoryResolver ?? function () {


yceruto · 2018-04-19T19:46:16Z

src/Symfony/Component/Messenger/Tests/Transport/Enhancers/MemoryLimitReceiverTest.php

+    public function memoryProvider()
+    {
+        return array(
+            array(2048, 1024, true),


sroze · 2018-04-20T07:55:15Z

src/Symfony/Component/Messenger/Tests/Transport/Enhancers/MemoryLimitReceiverTest.php

+    }
+}
+
+class ReceiverToDecorate implements ReceiverInterface


Could you instead use the CallbackReceiver (that needs to be moved to a proper file) ?

sroze · 2018-04-20T07:58:07Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+                $size = $matches[1] * 1024;
+            }
+        } else {
+            throw new \InvalidArgumentException('Invalid memory limit given.');


Can you throw first to reduce a depth level? i.e. if (!\preg_match(...)) { thow /* ... */; }

sroze · 2018-04-20T08:00:56Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+
+    public function stop(): void
+    {
+        $this->decoratedReceiver->stop();


Actually, it is probably a good idea to log such behavior (no exception). Maybe an optional LoggerInterface argument that would be called with info when that happens? (note that it wouldn't be on this line but when you actually detect the memory usage line 41 of this line).

@lyrixx any PoV on the logging level?

sroze · 2018-04-20T12:16:41Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+            if ($memoryResolver() >= $this->memoryLimit) {
+                $this->stop();
+                if ($this->logger) {
+                    $this->logger->info('Receiver stopped due to memory limit exceeded.');


$this->logger !== null && $this->logger->info('Receiver stopped due to memory limit of {limit} exceeded', array('limit' => $this->memoryLimit));

sroze · 2018-04-20T12:16:58Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+    {
+        if (!\preg_match('/^(\d+)([G|M|K]*)$/', $size, $matches)) {
+            throw new \InvalidArgumentException('Invalid memory limit given.');
+        } else {


Well, you don't need the else anymore :)

sroze · 2018-04-20T13:04:10Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+    private $memoryResolver;
+
+    public function __construct(
+        ReceiverInterface $decoratedReceiver,


Also, we don’t use multi-line constructors in Symfony

sroze · 2018-04-20T14:47:23Z

The tests are failing. Looks like some infinite loop because the tests for the component just never ends. Can you look at it? Also, I believe we should use inject the logger within the decorators now that it's possible: inject it to the command and to the receivers.

sdelicata · 2018-04-20T17:50:34Z

I've reproduced it. I'll try to fix soon

sdelicata · 2018-04-20T19:27:02Z

Test fixed.
MaximumCounterReceiver doesn't increase its internal counter anymore when receiving null message, so it will never stop :p

sdelicata · 2018-04-20T19:36:01Z

Also, I believe we should use inject the logger within the decorators now that it's possible: inject it to the command and to the receivers
@sroze That's exactly what I planed ;)

yceruto · 2018-04-20T20:52:21Z

src/Symfony/Component/Messenger/Tests/Adapter/AmqpExt/AmqpExtIntegrationTest.php

            $this->assertNull($message);
+
+            if (2 == ++$receivedMessages) {


strict comparison === ?

ping @sdelicata

yceruto · 2018-04-20T21:01:52Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+            $size = $matches[1] * 1024 * 1024 * 1024;
+        } elseif ('M' == $matches[2]) {
+            $size = $matches[1] * 1024 * 1024;
+        } elseif ('K' == $matches[2]) {


same here and above.

Tobion · 2018-04-20T23:27:27Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+            $handler($message);
+
+            $memoryResolver = $this->memoryResolver;
+            if ($memoryResolver() >= $this->memoryLimit) {


the limit is described as

Use the --memory-limit option to stop the worker if it exceeds a given memory usage limit

"exceed" means more than the limit, so >, not >=

Tobion · 2018-04-20T23:37:39Z

src/Symfony/Component/Messenger/Tests/Transport/Enhancers/MemoryLimitReceiverTest.php

+    /**
+     * @dataProvider memoryProvider
+     */
+    public function testReceiverStopsWhenMemoryLimitExceeded($memoryUsage, $memoryLimit, $shouldStop)


please add type declarations

Changes to be made

sroze · 2018-04-22T10:24:46Z

Status: Needs work

ostrolucky · 2018-04-22T11:11:24Z

I don't like naming of these. IMO it should signify what's being done when limit is exceeded. Otherwise, there will be confusion when somebody implements receiver with same trigger but another action. Also, I had no idea what it does until I looked into code. I thought it will just wait until memory is freed up or it will try to release some memory.

MaximumCountReceiver -> AbortWhenMaximumCountIsExceededReceiver
MemoryLimitReceiver -> AbortWhenMaximumMemoryIsExceededReceiver

sroze · 2018-04-22T11:45:35Z

That's a fair point you have here @ostrolucky. Though, no need to "maximum" if we already have "exceeded". Also, the receivers have this notion of "stop", so I'd suggest:

MaximumCountReceiver -> StopWhenMessageCountIsExceededReceiver
MemoryLimitReceiver -> StopWhenMemoryUsageIsExceededReceiver

fabpot · 2018-04-22T16:11:18Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+        $this->decoratedReceiver->stop();
+    }
+
+    private function convertToOctets(string $size): int


I fully agree.

fabpot · 2018-04-22T16:12:35Z

src/Symfony/Component/Messenger/Transport/Enhancers/MemoryLimitReceiver.php

+        $this->decoratedReceiver->stop();
+    }
+
+    private function convertToOctets(string $size): int


Implementation should be copied from existing code where we already have such logic in different components (see Symfony\Component\HttpKernel\DataCollector\MemoryDataCollector::convertToBytes for instance).

sroze · 2018-04-23T08:04:11Z

src/Symfony/Component/Messenger/Tests/Adapter/AmqpExt/AmqpExtIntegrationTest.php

            $this->assertNull($message);
+
+            if (2 == ++$receivedMessages) {


ping @sdelicata

sroze · 2018-04-23T08:05:14Z

.../Component/Messenger/Tests/Transport/Enhancers/StopWhenMemoryUsageIsExceededReceiverTest.php

+        $logger = $this->createMock(LoggerInterface::class);
+        $logger->expects($this->once())->method('info')
+            ->with(
+                $this->equalTo('Receiver stopped due to memory limit of {limit} exceeded'),


As FYI, you don't need $this->equalTo. This would work:

->with('Receiver stopped due to memory limit of {limit} exceeded', array('limit' => 64 * 1024 * 1024));

ro0NL · 2018-04-23T09:42:08Z

src/Symfony/Component/Messenger/Transport/Enhancers/StopWhenMemoryUsageIsExceededReceiver.php

+        $this->decoratedReceiver = $decoratedReceiver;
+        $this->memoryLimit = $memoryLimit;
+        $this->logger = $logger;
+        $this->memoryResolver = $memoryResolver ?: function () {


ro0NL · 2018-04-23T09:42:16Z

src/Symfony/Component/Messenger/Tests/Fixtures/CallbackReceiver.php

+
+    public function receive(callable $handler): void
+    {
+        $callable = $this->callable;


($this->callable)($handler);

sroze · 2018-04-25T16:26:38Z

@fabpot could you update your review please?

sroze · 2018-04-25T16:28:42Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

+
+    private function convertToBytes($memoryLimit)
+    {
+        if ('-1' === $memoryLimit) {


I really don't get the point of this -1 here. I appreciate it's coming from a copy/pasted code somewhere else but I'd remove it.

Tobion · 2018-04-25T21:16:00Z

@sdelicata please squash your commits into a single commit. some commit seems to be authored with a different email, so we cannot squash them.

Tobion · 2018-04-25T21:16:19Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

        }

        $worker = new Worker($receiver, $this->bus);
        $worker->run();
    }
+
+    private function convertToBytes($memoryLimit)


missing types

sdelicata · 2018-04-26T08:32:44Z

@Tobion Squash done

Tobion · 2018-04-26T23:14:54Z

Thanks for your work on this new feature!

…agesCommand` (sdelicata) This PR was merged into the 4.1-dev branch. Discussion ---------- [Messenger] Add a memory limit option for `ConsumeMessagesCommand` | Q | A | ------------- | --- | Branch? | master for features | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT New feature to add a memory limit option to the ConsumeMessagesCommand. ``` bin/console messenger:consume-messages <receiver-name> -m 128 ``` Commits ------- 08f98cf [Messenger] Add a memory limit option for `ConsumeMessagesCommand`

carsonbot added Status: Needs Review Feature labels Apr 18, 2018

linaori reviewed Apr 18, 2018

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Apr 18, 2018

sroze added the Messenger label Apr 18, 2018

sroze suggested changes Apr 18, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 18, 2018

sdelicata force-pushed the feat_memory_limit_consumer branch 2 times, most recently from 5a8f69f to 68582dd Compare April 19, 2018 15:57

yceruto reviewed Apr 19, 2018

View reviewed changes

sroze reviewed Apr 20, 2018

View reviewed changes

sdelicata force-pushed the feat_memory_limit_consumer branch 2 times, most recently from 0d2d782 to c1236ce Compare April 20, 2018 12:17

sroze reviewed Apr 20, 2018

View reviewed changes

sdelicata force-pushed the feat_memory_limit_consumer branch 2 times, most recently from 0a8460d to 37ee3a2 Compare April 20, 2018 13:11

sroze previously approved these changes Apr 20, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 20, 2018

sroze mentioned this pull request Apr 20, 2018

[Messenger] Avoid counter increase when receiving null message #26974

Closed

sdelicata force-pushed the feat_memory_limit_consumer branch from f836b55 to a4f1205 Compare April 20, 2018 19:23

sdelicata force-pushed the feat_memory_limit_consumer branch from 0babb6d to b13adcb Compare April 20, 2018 20:31

yceruto reviewed Apr 20, 2018

View reviewed changes

Tobion reviewed Apr 20, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Apr 22, 2018

fabpot requested changes Apr 22, 2018

View reviewed changes

sdelicata force-pushed the feat_memory_limit_consumer branch 2 times, most recently from 6826842 to f8e8125 Compare April 22, 2018 17:53

carsonbot removed the Status: Needs Work label Apr 23, 2018

sroze approved these changes Apr 23, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Apr 23, 2018

sdelicata force-pushed the feat_memory_limit_consumer branch from f8e8125 to 8b8b368 Compare April 23, 2018 08:39

ro0NL reviewed Apr 23, 2018

View reviewed changes

sroze reviewed Apr 25, 2018

View reviewed changes

Tobion approved these changes Apr 25, 2018

View reviewed changes

Tobion reviewed Apr 25, 2018

View reviewed changes

sdelicata force-pushed the feat_memory_limit_consumer branch from e733ea7 to 7fe7738 Compare April 26, 2018 08:28

[Messenger] Add a memory limit option for ConsumeMessagesCommand

08f98cf

sdelicata force-pushed the feat_memory_limit_consumer branch from 7fe7738 to 08f98cf Compare April 26, 2018 08:29

Tobion merged commit 08f98cf into symfony:master Apr 26, 2018

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged


		Use the --memory option to limit the memory consumed by the worker:

		<info>php %command.full_name% <receiver-name> --memory=128</info>

[Messenger] Add a memory limit option for ConsumeMessagesCommand #26975

[Messenger] Add a memory limit option for ConsumeMessagesCommand #26975

Conversation

sdelicata commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Apr 20, 2018

sdelicata commented Apr 20, 2018

sdelicata commented Apr 20, 2018

sdelicata commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Apr 22, 2018

ostrolucky commented Apr 22, 2018

sroze commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Apr 25, 2018

Choose a reason for hiding this comment

sdelicata commented Apr 26, 2018

Tobion commented Apr 26, 2018

[Messenger] Add a memory limit option for `ConsumeMessagesCommand` #26975

[Messenger] Add a memory limit option for `ConsumeMessagesCommand` #26975

sroze commented Apr 18, 2018 •

edited

Loading

yceruto Apr 19, 2018 •

edited

Loading

sroze Apr 20, 2018 •

edited

Loading