prolic / HumusAmqp

PHP 7.4 AMQP library
https://humusamqp.readthedocs.io/
MIT License
76 stars 17 forks source link

Next #110

Closed basz closed 1 year ago

basz commented 1 year ago

connection works now. could have a quick look at these errors (7.4) https://github.com/basz/HumusAmqp/actions/runs/6191320600/job/16809411289

basz commented 1 year ago

narrowed it down to really take care of removing existing queues n exchanges after tests.

I did skip a few problematic tests for now, but matrix is all green on 7.4 - 8.2.

https://github.com/basz/HumusAmqp/actions/runs/6199696178

basz commented 1 year ago

only amqp it_handles_flush_deferred_after_timeout test is skipped now due to an error. I can't figure it out. It will not pass. would appreciate your time on this one.

docker run --rm --name rabbitmq --publish 5672:5672 --publish 5671:5671 --publish 5050:15672 --volume ./provision/test_certs:/rabbitmq-certs:ro --volume ./provision/rabbitmq.config:/etc/rabbitmq/rabbitmq.conf:ro --volume ./provision/definitions.json:/etc/rabbitmq/definitions.json:ro -e RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS="-rabbitmq_management load_definitions '/etc/rabbitmq/definitions.json'" rabbitmq:3-management.2:5672

runs rabbitmq so you can test locally

func0der commented 1 year ago

I just wanted to let you know, that I can not help you out with this in a timely manner. Did not want to let you be without any feedback though :)

prolic commented 1 year ago

@basz I checked your failing test it_handles_flush_deferred_after_timeout

I'm not 100% sure yet, but I would try to following:

And then test again. Maybe this is really a protocol issue that is resolved when you install latest versions.

prolic commented 1 year ago

For reference, easier to read:

docker run --rm \
--name rabbitmq \
--publish 5672:5672 \
--publish 5671:5671 \
--publish 5050:15672 \
--volume ./provision/test_certs:/rabbitmq-certs:ro \
--volume ./provision/rabbitmq.config:/etc/rabbitmq/rabbitmq.conf:ro \
--volume ./provision/definitions.json:/etc/rabbitmq/definitions.json:ro \
-e RABBITMQ_LOAD_DEFINITIONS="/etc/rabbitmq/definitions.json" \
rabbitmq:3-management
basz commented 1 year ago

It seems to be introduced in rabbitmq:3.10.8 as 3.10.7 passes. switching between php-amqp 1 or 2 does not seem to matter. neither does installing from source. I am sure installing rabbitmq from source will help in this case.

https://github.com/rabbitmq/rabbitmq-server/compare/v3.10.7...v3.10.8 https://github.com/rabbitmq/rabbitmq-server/blob/627b29c3a582352fee8bc6277b05541ed05846bf/release-notes/3.10.8.md

rabbitmq logging (locally) reports an initial error with 'client unexpectedly closed TCP connection'. Which pops up in PHP as a 'Protocol error'... I mention this because exchange->delete() is called explicitly in the test. Maybe it is not allowed on that moment... I do not understand what the test is suppost to test.

I updated the test matrix to reflect the above... here are the latest results https://github.com/basz/HumusAmqp/actions/runs/6246293544/job/16956630938

basz commented 1 year ago

@prolic, I found a solution I would like you to consider. In that particular test an exchange is deleted and therefore an new channel is created to setup some stuff. However that is new channel is created from the existing connection. I noticed the error disappears when I create a new connection first.

So... I conclude that even while the existing connection reports it is connected something is at that time wrong with it in versions >= 3.10.8. (protocol state)

While this fixes the tests do you think it is an acceptable solution for now? Should the library try to recover or we should leave that to end user?

basz commented 1 year ago

While this fixes the tests do you think it is an acceptable solution for now? Should the library try to recover or we should leave that to end user?

I'm going to merge this as is. I think it is ok if the library does not try to work around issues with magic. If you disagree just kick me.

prolic commented 1 year ago

Since it's only happening with amp extension and not with phpamqplib, I think open a ticket on the PHP extension first. Maybe that's something they can resolve.

lost-anjarrett commented 1 year ago

@basz thank you, great job. Sorry for the lack of help, not much time to give myself. I'll test it in next days/week and give a feedback