New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Apr 26, 2018

Conversation

@sdelicata
Contributor

sdelicata commented Apr 18, 2018

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

This comment has been minimized.

@iltar

iltar Apr 18, 2018

Contributor

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?

This comment has been minimized.

@sdelicata

sdelicata Apr 18, 2018

Contributor

Good idea, I did it

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

@sroze sroze added the Messenger label Apr 18, 2018

@sroze

This comment has been minimized.

Member

sroze commented Apr 18, 2018

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

@@ -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]:

This comment has been minimized.

@sroze

sroze Apr 18, 2018

Member
- 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]:

This comment has been minimized.

@sdelicata

sdelicata Apr 19, 2018

Contributor

Changed

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

This comment has been minimized.

@sroze

sroze Apr 18, 2018

Member

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

This comment has been minimized.

@sdelicata

sdelicata Apr 19, 2018

Contributor

Done

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

This comment has been minimized.

@yceruto

yceruto Apr 19, 2018

Contributor

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

This comment has been minimized.

@sdelicata

sdelicata Apr 20, 2018

Contributor

True, unnecessary anymore, thx

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

This comment has been minimized.

@yceruto

yceruto Apr 19, 2018

Contributor

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?

This comment has been minimized.

@sdelicata

sdelicata Apr 20, 2018

Contributor

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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?

This comment has been minimized.

@sdelicata

sdelicata Apr 20, 2018

Contributor

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

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

This comment has been minimized.

@yceruto

yceruto Apr 19, 2018

Contributor

?: ?

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

This comment has been minimized.

@yceruto

yceruto Apr 19, 2018

Contributor

yield?

}
}
class ReceiverToDecorate implements ReceiverInterface

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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

This comment has been minimized.

@sdelicata

sdelicata Apr 20, 2018

Contributor

Of course

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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?

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member
$this->logger !== null && $this->logger->info('Receiver stopped due to memory limit of {limit} exceeded', array('limit' => $this->memoryLimit));
{
if (!\preg_match('/^(\d+)([G|M|K]*)$/', $size, $matches)) {
throw new \InvalidArgumentException('Invalid memory limit given.');
} else {

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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

This comment has been minimized.

@sdelicata

sdelicata Apr 20, 2018

Contributor

😆

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

This comment has been minimized.

@sroze

sroze Apr 20, 2018

Member

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

@sroze

This comment has been minimized.

Member

sroze commented Apr 20, 2018

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

This comment has been minimized.

Contributor

sdelicata commented Apr 20, 2018

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

@sdelicata

This comment has been minimized.

Contributor

sdelicata commented Apr 20, 2018

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

@sdelicata

This comment has been minimized.

Contributor

sdelicata commented Apr 20, 2018

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 ;)

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

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

strict comparison === ?

This comment has been minimized.

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

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

same here and above.

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

This comment has been minimized.

@yceruto

yceruto Apr 20, 2018

Contributor

Problably this will be useful for debugging purpose, so ->debug() fits better?

This comment has been minimized.

@sroze

sroze Apr 21, 2018

Member

I believe this is more than debug as it highlights an issue (i.e. too much memory consumed). So info is, to me, the ideal level.

$size = $matches[1] * 1024;
}
return $size;

This comment has been minimized.

@Tobion

Tobion Apr 20, 2018

Member

this should be cast to int explicitly to avoid errors with strict typing

This comment has been minimized.

@ostrolucky

ostrolucky Apr 22, 2018

Contributor

What do you mean? Symfony doesn't use strict typings so this would be casted to int implicitly anyway. This method will be removed anyway though so it doesn't matter now.

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

This comment has been minimized.

@Tobion

Tobion Apr 20, 2018

Member

IMO it's not the responsibility of this class to accept user-friendly inputs. it should only accept bytes. the transformation should happen where the input is entered, i.e. in the ConsumeMessagesCommand

This comment has been minimized.

@fabpot

fabpot Apr 22, 2018

Member

I fully agree.

This comment has been minimized.

@fabpot

fabpot Apr 22, 2018

Member

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).

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

This comment has been minimized.

@Tobion

Tobion Apr 20, 2018

Member

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 >=

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

This comment has been minimized.

@Tobion

Tobion Apr 20, 2018

Member

please add type declarations

@sroze sroze dismissed their stale review Apr 21, 2018

Changes to be made

@sroze

This comment has been minimized.

Member

sroze commented Apr 22, 2018

Status: Needs work

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Apr 22, 2018

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

This comment has been minimized.

Member

sroze commented Apr 22, 2018

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
$this->decoratedReceiver->stop();
}
private function convertToOctets(string $size): int

This comment has been minimized.

@fabpot

fabpot Apr 22, 2018

Member

I fully agree.

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

This comment has been minimized.

@fabpot

fabpot Apr 22, 2018

Member

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

sroze approved these changes Apr 23, 2018

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

This comment has been minimized.

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

This comment has been minimized.

@sroze

sroze Apr 23, 2018

Member

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));
$this->decoratedReceiver = $decoratedReceiver;
$this->memoryLimit = $memoryLimit;
$this->logger = $logger;
$this->memoryResolver = $memoryResolver ?: function () {

This comment has been minimized.

@ro0NL

ro0NL Apr 23, 2018

Contributor

??

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

This comment has been minimized.

@ro0NL

ro0NL Apr 23, 2018

Contributor

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

@sroze

This comment has been minimized.

Member

sroze commented Apr 25, 2018

@fabpot could you update your review please?

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

This comment has been minimized.

@sroze

sroze Apr 25, 2018

Member

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.

This comment has been minimized.

@sdelicata

sdelicata Apr 25, 2018

Contributor

I agree

@Tobion

Tobion approved these changes Apr 25, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 25, 2018

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

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

This comment has been minimized.

@Tobion

Tobion Apr 25, 2018

Member

missing types

@sdelicata

This comment has been minimized.

Contributor

sdelicata commented Apr 26, 2018

@Tobion Squash done

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 26, 2018

Thanks for your work on this new feature!

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

Tobion added a commit that referenced this pull request Apr 26, 2018

feature #26975 [Messenger] Add a memory limit option for `ConsumeMess…
…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`

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment