-
Notifications
You must be signed in to change notification settings - Fork 106
[0.6.x] Make it abundantly clear Bunny must run in a fiber #193
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
[0.6.x] Make it abundantly clear Bunny must run in a fiber #193
Conversation
|
Thank you so much for extending the documentation! What I was looking for was a way to upgrade |
|
I think I saw "Cannot switch fibers in current execution context" when I was trying to shut my service down in a signal handler, I take it in that situation it is running outside of a fiber context? I think I fixed it by calling my shutdown code in a Loop::futureTick(). I wasn't sure if I was doing things the "correct" way but it looks like I may have guessed correctly? :-) |
That's also exactly my case, terminating a worker via signal handler: Within the |
andrew-demb
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.
This is great, thank you. I left some suggestions to improve it a bit from my PoV
README.md
Outdated
| $bunny->connect(); // No perse needed, as the Client::channel() method also does this, but added for completeness sake | ||
| $channel = $bunny->channel(); | ||
| $channel->queueDeclare('queue_name'); // Queue name | ||
| $channel->close(); // No perse needed, as the Client::disconnect() method also does this, but added for completeness sake |
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.
What does it mean "No perse needed"? I'm not sure about the translation.
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 spotted this, I think he may have meant "Not needed per se, as the Client..."?
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 suggest rephrasing it then to make it clear 👍
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.
So, the client will open a connection when you try to open a channel if it's not connected or in the process of connecting:
Lines 155 to 170 in 5a37f76
| /** | |
| * Creates and opens new channel. | |
| * | |
| * Channel gets first available channel id. | |
| */ | |
| public function channel(): ChannelInterface | |
| { | |
| if (!$this->isConnected()) { | |
| $this->connect(); | |
| } | |
| if ($this->state === ClientState::Connecting) { | |
| $this->awaitConnection(); | |
| } | |
| $channelId = $this->findChannelId(); |
Technically, that means there is no need to call Client::connect() directly, making Bunny on the connecting side lazy.
Similarly, when calling Client::disconnect(), it will close all open channels
Lines 290 to 297 in 5a37f76
| $promises = []; | |
| foreach ($this->channels->all() as $channelId => $channel) { | |
| $promises[] = async(static function () use ($channel, $replyCode, $replyText): void { | |
| $channel->close($replyCode, $replyText); | |
| })(); | |
| } | |
| await(all($promises)); |
Channel::close() when disconnecting.
Updated the message a bit, but I can also remove them.
| ); | ||
|
|
||
| $bunny = new Client($configuration); | ||
| Loop::futureTick(async(static function (): void { |
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.
Why is it necessary to use Loop::futureTick, but not async? I'm not sure whether it is a necessary detail to be known by the developer for classic use cases (php-fpm with no full async application)
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.
Why is it necessary to use Loop::futureTick, but not async?
Loop::futureTick isn't strictly necessary, async is tho. You could do async(static fn etc etc)() to start the fiber right away, but using a Loop::futureTick schedules it right after the event loop has finished all it's incoming working from the kernel.
I'm not sure whether it is a necessary detail to be known by the developer for classic use cases (php-fpm with no full async application)
It shouldn't matter to them. Will have a look at adding a section for that tomorrow.
b6d19cf to
0b7cdbe
Compare
Correct, and @andrew-demb also made some good points on this topic. Haven't been able to respond to everything, but pushed some changes based on suggestions. |
0b7cdbe to
6351b75
Compare
Just pushed updated quickstart examples that cover this, also added the logic to both consumer examples in the readme. |
6351b75 to
c5d046e
Compare
Found this while working on #193
Found this while working on #193
c5d046e to
eff8c15
Compare
Found this while working on #193
Found this while working on #193
Found this while working on #193
Found this while working on #193
Found this while working on #193
eff8c15 to
69c683d
Compare
@pulmer-cp @mdissington @andrew-demb @menturion @kostirez1 Since you either created or responded/emoted to some message in #192 or #191 I would love your feedback on this PR, addressing that in the documentation and examples. The changes in the readme start here: https://github.com/jakubkulhan/bunny/tree/0.6.x-make-it-abundantly-clear-Bunny-must-run-in-a-fiber?tab=readme-ov-file#always-run-moving-parts-in-a-fiber