Skip to content
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

Next #110

Merged
merged 37 commits into from
Sep 21, 2023
Merged

Next #110

merged 37 commits into from
Sep 21, 2023

Conversation

basz
Copy link
Collaborator

@basz basz commented Sep 14, 2023

No description provided.

@basz basz changed the title [WIP Next [WIP] Next Sep 14, 2023
@basz
Copy link
Collaborator Author

basz commented Sep 14, 2023

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

@basz
Copy link
Collaborator Author

basz commented Sep 15, 2023

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
Copy link
Collaborator Author

basz commented Sep 16, 2023

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
Copy link
Contributor

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
Copy link
Owner

prolic commented Sep 19, 2023

@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
Copy link
Owner

prolic commented Sep 19, 2023

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
Copy link
Collaborator Author

basz commented Sep 20, 2023

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.

rabbitmq/rabbitmq-server@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
Copy link
Collaborator Author

basz commented Sep 20, 2023

@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 basz mentioned this pull request Sep 21, 2023
@basz basz changed the title [WIP] Next Next Sep 21, 2023
@basz
Copy link
Collaborator Author

basz commented Sep 21, 2023

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.

@basz basz merged commit e87d034 into prolic:master Sep 21, 2023
@prolic
Copy link
Owner

prolic commented Sep 21, 2023

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
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants