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

Channel.basic_publish does not adhere to it's return type. #183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MaPePeR
Copy link

@MaPePeR MaPePeR commented Jun 2, 2023

Specify that ConfirmationType Future contains ConfirmationFrameType.

Channel.confirmations contains the Futures used by basic_publish, so the Future needs to adhere to the return type of basic_publish.

Setting the type reveals an error in Channel._on_return_frame, which resolves the Future to DeliveredMessage, which is not a ConfirmationFrameType, thus breaking it's contract.

Not sure how to proceed here, tbh., because fixing this can probably be considered a BC-break at this point?

The DeliveredMessage that is wrongly returned by basic_publish can be reproduced by trying to publish to the default_exchange with a routing_key that doesn't match any queue and having on_return_raises=False (which for some reason is the default in aio-pika? 😳).

#100 might be related to this.

…ype`.

`Channel.confirmations` contains the `Future`s used by `basic_publish`,
so the `Future` needs to adhere to the return type of `basic_publish`.

This reveals an error in `Channel._on_return_frame`, which resolves the
`Future` to `DeliveredMessage`, which is not a `ConfirmationFrameType`
@mosquito
Copy link
Owner

mosquito commented Jun 2, 2023

This not compatible for older python versions as you can see.

@MaPePeR
Copy link
Author

MaPePeR commented Jun 2, 2023

Latest commit should fix the incompatibility with old python versions. Sadly had to resort to sys.version_info. I would have preferred hasattr(asyncio.Future, '__getitem__'), but mypy didn't understand that.

This avoids the check for sys.version_info.
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.

2 participants