jakubkulhan / bunny

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

fix closing connection on server shutdown #100

Closed Tatikoma closed 4 years ago

Tatikoma commented 4 years ago

Hello, currently bunny incorrectly handling rabbitmq server shutdown.

Way to reproduce:

  1. create async Bunny client instance and connect to rabbitmq server
  2. stop rabbitmq application with rabbitmqctl stop_app

Expected result ClientException, that i can handle and call $client->disconnect().

Actual result ClientException, but when i try to call $client->disconnect() it produces new exception:

Broken pipe or closed connection.
#0 /home/tatikoma/PhpstormProjects/project/vendor/bunny/bunny/src/Bunny/Async/Client.php(297): Bunny\AbstractClient->read()
#1 /home/tatikoma/PhpstormProjects/project/vendor/amphp/react-adapter/src/ReactAdapter.php(44): Bunny\Async\Client->onDataAvailable()
#2 /home/tatikoma/PhpstormProjects/project/vendor/amphp/amp/lib/Loop/NativeDriver.php(183): Amp\ReactAdapter\ReactAdapter::Amp\ReactAdapter\{closure}()
#3 /home/tatikoma/PhpstormProjects/project/vendor/amphp/amp/lib/Loop/NativeDriver.php(96): Amp\Loop\NativeDriver->selectStreams()
#4 /home/tatikoma/PhpstormProjects/project/vendor/amphp/amp/lib/Loop/Driver.php(138): Amp\Loop\NativeDriver->dispatch()
#5 /home/tatikoma/PhpstormProjects/project/vendor/amphp/amp/lib/Loop/Driver.php(72): Amp\Loop\Driver->tick()
#6 /home/tatikoma/PhpstormProjects/project/vendor/amphp/amp/lib/Loop.php(95): Amp\Loop\Driver->run()
#7 /home/tatikoma/PhpstormProjects/project/src/project/project/Async/Server.php(166): Amp\Loop::run()
#8 /home/tatikoma/PhpstormProjects/project/bin/server.php(11): Project\Project\Async\Server->listen()
#9 {main}

This way client never run to state ClientStateEnum::NOT_CONNECTED and any attempts to close connection will produce new exceptions.

I using it with amphp with ReactAdapter, but i think it doesnt matter. This patch cant break something, so i guess it can be easily merged to upstream.

PS: $client->disconnect() is only place where "$this->eventLoop->removeReadStream($this->getStream());" is located. So i think that i should call this method when server closes connection.

Tatikoma commented 4 years ago

I also added code to disconnect client from server, if server forced connection close. But i saved exception for backward compatability.

I working on cluster connection wrapper and this fix is important for me. I have multiple connections to rabbitmq in case of failures. If i dont close connection, then i dont know which client produced exception and cant handle. This way i just checking isConnected().

WyriHaximus commented 4 years ago

Way to reproduce:

  1. create async Bunny client instance and connect to rabbitmq server
  2. stop rabbitmq application with rabbitmqctl stop_app

Would be nice to add tests for this in a follow up PR

I working on cluster connection wrapper and this fix is important for me. I have multiple connections to rabbitmq in case of failures. If i dont close connection, then i dont know which client produced exception and cant handle. This way i just checking isConnected().

Are you making that into a package? Could be useful

Tatikoma commented 4 years ago

Sorry, i'm not enough familiar with testing. Just checking my code plenty times.

Cluster wrapper is packaged, but i will ask my chief if i can publish it.

WyriHaximus commented 4 years ago

@Tatikoma no worries, will have a look soon. If you chief does let you publish it, please ping me here or open an issue. Alternatively, we can consider adding it to this package