Throw connection exceptions on batch publish

[foment]

Hello,

in bulk publishing, the connection existence is not checked (PhpAmqpLib/Channel/AMQPChannel.php:1232 in publish_batch method) and therefore we can't catch proper exception, like for example AMQPChannelClosedException.

This sometimes happens when there is a delay between opening the connection and actual publishing and for example in the meantime connection was closed for any reason whatsoever.

Therefore, here is the suggestion to throw a proper exception already in the prepare_method_frame method, like it's done for send_method_frame method in the AbstractChannel class.

[foment]

Hi,
any update on this PR?

Cheers :)

[ramunasd]

Hi @foment , thanks for pointing this out and your efforts. Can You explain why You picked method prepare_method_frame ? This one is called only from prePublish() and only when "publish cache" is not warmed up.
Isn't better to check connection in publish_batch() ? Well, there is one check, maybe it's better to move it to beginning of method?
Anyways, this check does not garantuee stable connection. I would like to suggest enabling keepalive and heartbeat with signal senders. They are doing great job when scripts aren't responsive enough.

[foment]

Hi @ramunasd,

thank you for the quick reply.

The idea behind throwing an exception in prepare_method_frame is more to be aligned with the current implementation like in send_method_frame for example, where connection is checked for it's existence. You're right, as an alternative, I could move $this->checkConnection(); call in the publish_batch method to the beginning (before actual call to prePublish method).

One of the reasons behind all of this is to be able to catch proper exception in one of the legacy projects I'm currently working on and in general to have proper connection check before actually using it (like in publish_batch), which leads to php fatal error.

[ramunasd]

So I don't see reason why "packet" method should be aware about connection. Please follow my suggestion. Thanks

[foment]

PR updated with implemented suggestion.

[codecov]

Codecov Report

Merging #983 (9684db1) into master (1718c20) will increase coverage by 0.85%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #983      +/-   ##
============================================
+ Coverage     73.53%   74.39%   +0.85%     
  Complexity      982      982              
============================================
  Files            35       35              
  Lines          2675     2675              
============================================
+ Hits           1967     1990      +23     
+ Misses          708      685      -23     

Impacted Files	Coverage Δ
PhpAmqpLib/Channel/AMQPChannel.php	57.98% <100.00%> (+5.39%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1718c20...9684db1. Read the comment docs.

[ramunasd]

We still use older version of phpunit :)

[foment]

Fix is on the way :)