Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
WIP: [Messenger] Infinite Loop Receiver#28547
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
- Add a receiver decorator for standardizing the infinite looping format- Updated the AMQP transport to utilize the infinite looping receiver to simply designSigned-off-by: RJ Garcia <rj@bighead.net>
sroze left a comment
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 happy to merge if we don't introduce a BC break (i.e.AmqpReceiver using this abstraction) and that you have a clear use-case you can explain for this abstraction :)
| privatefunctiongetReceiver() | ||
| { | ||
| return$this->receiver =newAmqpReceiver($this->connection,$this->serializer); | ||
| return$this->receiver =newInfiniteLoopReceiver(newAmqpReceiver($this->connection,$this->serializer)); |
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.
That's the only issue I can see here: it's more painful to create the receiver and it's very much error-prone for whoever is using these objects directly. Can't theAmqpReceiver use theInfiniteLoopReceiver instead? 🤔
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.
@sroze I'm not sure, the only way for Amqp to utilize this would be if we provided a utility method like:
function performOnInfiniteLoop(callable $shouldStop, callable $handler) { while (!$shouldStop()) { $handler(); }}Also, are we 100% sure it's a bc break? because it's an internal class that's not really exposed to the outside since the TransportFactory is responsible for creating the actual receiver.
| * Stop receiving some messages. | ||
| */ | ||
| publicfunctionstop():void { | ||
| $this->shouldStop =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.
I guess you should call$this->receiver->stop() as well here.
ragboyjr commentedSep 22, 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.
Hmm, I’m not entirely sure how the AMQP receiver would use the infinite loop receiver. I it would only work as a decorator. The only other thing would be using inheritance, but I think that defeats the purpose of having a simple interface. The other option for BC is that we don’t touch the AMQP receiver, and just add the infinite loop one when that other receivers can use. Regarding a clear use case, I’m building a redis receiver for 4.1, and the receiver follows almost exactly the same pattern as the AMQP ext one. In fact everything is about the same including the connection. It seems like there are quite a few areas we could standardize, but felt like this would be a simple least obstructive one to make. Also, I’m trying to think of a use case for a receiver that wouldn’t want infinite looping/sleeping. The other option would be that the receiver just stops and the process ends, and you’d need to use something like supervisor or cron to keep that command running at intervals, but in that case they might be better off as a normal symfony command...
EDIT: Actually stop is needed to let the current receiver shut down properly. And on top of that the pcntl calls in Worker.php and InfiniteLoopReceiver could probably be better suited in their own Receiver like PcntlReceiver... |
ragboyjr commentedSep 22, 2018
Sorry I know this is a lot, but this is the last version to get the bc breaks in, so I’m just trying to bounce ideas off to see if they make any sense |
ragboyjr commentedSep 24, 2018
@sroze Added a test case for the InfiteLoopReceiver and also updated it to delegate stop to the inner receiver. |
Signed-off-by: RJ Garcia <rj@bighead.net>
c369557 tofb8e57cCompareroukmoute commentedNov 6, 2018
up? |
ragboyjr commentedDec 8, 2018
@sroze Circling back to this. The other option with the infinite loop receiver would be that it's an abstract class that could be extended to provide the functionality. The tradeoffs I see is that are as follows: Inheritance:
Decoration:
After thinking further, i almost wonder if the decoration should take place in the ConsumeMessages command, and we add a flag for |
looping format
receiver to simply design
Signed-off-by: RJ Garciarj@bighead.net
The AMQP looping and pcntl_signal structure is actually pretty common among other receivers like Redis or SQS. This WIP PR abstracts out that logic into a decorator to simplify the design of the AMQP receiver and to also prevent the duplication of code across the third party receivers.
It's WIP because I haven't added or updated tests yet, I wanted to create this first to see if you guys (@sroze) even approve of this change before I make the effort to get this ready for merge.
Thanks!