jakubkulhan / bunny

Performant pure-PHP AMQP (RabbitMQ) sync/async (ReactPHP) library
MIT License
706 stars 102 forks source link

Added catch to __destruct to prevent fatal error #122

Closed jakubvojacek closed 2 years ago

jakubvojacek commented 2 years ago

Hello

please check the changes I made. I know it's far from being optimal as that will catch any logical error, not just client exception etc but the fatal error in the __destruct was quite hard to find - the application was returning all data in the request yet the HTTP status was 500.

I recently migrated to https://github.com/cloudamqp/amqproxy since php cannot keep the connection opened. After the migration around 30% of requests started to fail with https://github.com/jakubkulhan/bunny/blob/master/src/Bunny/AbstractClient.php#L311

Apart from my solution what would you suggest is the best way to treat exceptions in destructor?

bckp commented 2 years ago

Have same issue, and fixed it with extending Client class, and adding all required stuff manually. So catching exception and throwing it away, as closed connection is irelevat if you are in __destruct anyway.

bckp commented 2 years ago

I would love to see this merged @jakubkulhan @WyriHaximus , as this solves a problems to lot of people (like 80% of messages to my extension with bunny is about this exception that is impossible to catch)

jakubvojacek commented 2 years ago

actually, it was causing issues for me in consumers, restarting haproxy would close connections to rabbitmq and the consumer wouldnt open a new one and i had to restart them manually.

I "solved" this by

so merging it probably isnt great idea but it could be perhaps configurable somehow, for consumers to behave one way and for regular request another way?

bckp commented 2 years ago

we have our own extension for app, that can catch this broken pipe exception and reconnect again, but on app shutdown, for long running producers, this is issue... We have case, when we run consumer, that check for queue, and if he get message, parse it, decide to what exchange should he push it and then try to do it, but if you have silence on input queue for like 10 minutes, producer will disconnect from rabbit as rabbit close connection. When app try to send message, broken pipe shows, but that is OK, as we can catch this and reconnect, but if we do not want to send message but restart consumer, then it is problem, as restarting consumer couse another broken pipe exception, but this one is out of scope of app and you cant catch it, but it will end in app logs and app ends with error, this case should be solved by your commit.

WyriHaximus commented 2 years ago

While working on #106 I had that exception enough as well so handling this well is high on my list. Could you rebase on latest master so this PR will be tested?

jakubvojacek commented 2 years ago

As per my previous comment, I do not think anymore its a good idea to merge, let me close this MR

WyriHaximus commented 2 years ago

:+1: I'll look at a better/different solution to resolve this. Thanks again for putting the time into this