Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Add a memory limit option forConsumeMessagesCommand#26975
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| Use the --memory option to limit the memory consumed by the worker: | ||
| <info>php %command.full_name% <receiver-name> --memory=128</info> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 useG, M, K support here, just like with php?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea, I did it
sroze commentedApr 18, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Same as for the other PR, could you have some tests? :) |
| <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]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- 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]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed
| privatefunctionconvertToOctets(string$size):int | ||
| { | ||
| if (\preg_match('/^(\d+)(.)$/',$size,$matches)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd be stricter than.. Maybe just(G|M|K) and throw an exception if nothing is recognized to prevent any issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
5a8f69f to68582ddCompare| thrownew \InvalidArgumentException('Invalid memory limit given.'); | ||
| } | ||
| return (int)$size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could(int) casting be removed? seems unnecessary after\d+ and... * 1024 ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
True, unnecessary anymore, thx
| publicfunctionstop():void | ||
| { | ||
| $this->decoratedReceiver->stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see any reason throwing an exception for this. What is your use-case? Do you need to know this for something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, it is probably a good idea to log such behavior (no exception). Maybe an optionalLoggerInterface argument that would be called withinfo 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I disagree exception too, but the optional LoggerInterface is a good idea
| { | ||
| $this->decoratedReceiver =$decoratedReceiver; | ||
| $this->memoryLimit =$this->convertToOctets($memoryLimit); | ||
| $this->memoryResolver =$memoryResolver ??function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
?: ?
| publicfunctionmemoryProvider() | ||
| { | ||
| returnarray( | ||
| array(2048,1024,true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yield?
| } | ||
| } | ||
| class ReceiverToDecorateimplements ReceiverInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you instead usetheCallbackReceiver (that needs to be moved to a proper file) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Of course
| $size =$matches[1] *1024; | ||
| } | ||
| }else { | ||
| thrownew \InvalidArgumentException('Invalid memory limit given.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you throw first to reduce a depth level? i.e.if (!\preg_match(...)) { thow /* ... */; }
| publicfunctionstop():void | ||
| { | ||
| $this->decoratedReceiver->stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, it is probably a good idea to log such behavior (no exception). Maybe an optionalLoggerInterface argument that would be called withinfo 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?
0d2d782 toc1236ceCompare| if ($memoryResolver() >=$this->memoryLimit) { | ||
| $this->stop(); | ||
| if ($this->logger) { | ||
| $this->logger->info('Receiver stopped due to memory limit exceeded.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$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)) { | ||
| thrownew \InvalidArgumentException('Invalid memory limit given.'); | ||
| }else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, you don't need theelse anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
😆
| private$memoryResolver; | ||
| publicfunction__construct( | ||
| ReceiverInterface$decoratedReceiver, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, we don’t use multi-line constructors in Symfony
0a8460d to37ee3a2Comparesroze commentedApr 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 |
sdelicata commentedApr 20, 2018
I've reproduced it. I'll try to fix soon |
f836b55 toa4f1205Comparesdelicata commentedApr 20, 2018
Test fixed. |
sdelicata commentedApr 20, 2018
|
0babb6d tob13adcbCompare| $receiver->receive(function ($message)use ($receiver, &$receivedMessages) { | ||
| $this->assertNull($message); | ||
| if (2 == ++$receivedMessages) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
strict comparison=== ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ping@sdelicata
| $size =$matches[1] *1024 *1024 *1024; | ||
| }elseif ('M' ==$matches[2]) { | ||
| $size =$matches[1] *1024 *1024; | ||
| }elseif ('K' ==$matches[2]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same here and above.
| $handler($message); | ||
| $memoryResolver =$this->memoryResolver; | ||
| if ($memoryResolver() >=$this->memoryLimit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| */ | ||
| publicfunctiontestReceiverStopsWhenMemoryLimitExceeded($memoryUsage,$memoryLimit,$shouldStop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please add type declarations
sroze commentedApr 22, 2018
Status: Needs work |
ostrolucky commentedApr 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 |
sroze commentedApr 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:
|
| $this->decoratedReceiver->stop(); | ||
| } | ||
| privatefunctionconvertToOctets(string$size):int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I fully agree.
| $this->decoratedReceiver->stop(); | ||
| } | ||
| privatefunctionconvertToOctets(string$size):int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Implementation should be copied from existing code where we already have such logic in different components (seeSymfony\Component\HttpKernel\DataCollector\MemoryDataCollector::convertToBytes for instance).
6826842 tof8e8125Compare| $receiver->receive(function ($message)use ($receiver, &$receivedMessages) { | ||
| $this->assertNull($message); | ||
| if (2 == ++$receivedMessages) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ping@sdelicata
| $logger =$this->createMock(LoggerInterface::class); | ||
| $logger->expects($this->once())->method('info') | ||
| ->with( | ||
| $this->equalTo('Receiver stopped due to memory limit of {limit} exceeded'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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));
f8e8125 to8b8b368Compare| $this->decoratedReceiver =$decoratedReceiver; | ||
| $this->memoryLimit =$memoryLimit; | ||
| $this->logger =$logger; | ||
| $this->memoryResolver =$memoryResolver ?:function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
??
| publicfunctionreceive(callable$handler):void | ||
| { | ||
| $callable =$this->callable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
($this->callable)($handler);
sroze commentedApr 25, 2018
@fabpot could you update your review please? |
| privatefunctionconvertToBytes($memoryLimit) | ||
| { | ||
| if ('-1' ===$memoryLimit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree
Tobion commentedApr 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->run(); | ||
| } | ||
| privatefunctionconvertToBytes($memoryLimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing types
e733ea7 to7fe7738Compare7fe7738 to08f98cfComparesdelicata commentedApr 26, 2018
@Tobion Squash done |
Tobion commentedApr 26, 2018
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 | MITNew 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`
New feature to add a memory limit option to the ConsumeMessagesCommand.