Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

crash when delivery mode not specified in message #3

Closed
dmacnet opened this issue Oct 11, 2016 · 10 comments
Closed

crash when delivery mode not specified in message #3

dmacnet opened this issue Oct 11, 2016 · 10 comments

Comments

@dmacnet
Copy link

dmacnet commented Oct 11, 2016

I started testing compatibility between this module and the asynqp Python AMQP module. aio-pika crashes with a TypeError when reading a message that doesn't specify delivery mode, in message.py line 39:
self.delivery_mode = DeliveryMode(int(delivery_mode)).value
It fails to convert NoneType to an int.
I am not positive whether the delivery mode field of a message is required, but it would be nice to have either cleaner reporting of the problem or defaulting of the value in aio-pika, to check whether it is None before trying to convert it to an int.

@mosquito
Copy link
Owner

What you want to see by default when "None" was passed?

@mosquito mosquito reopened this Oct 13, 2016
@mosquito
Copy link
Owner

I think It's must be explicit and required.

@dmacnet
Copy link
Author

dmacnet commented Oct 14, 2016

OK, in that case I think I'd prefer that it log and discard malformed messages rather than raise an exception. Do you have a consistent approach to handling messages that are malformed in other ways?
I've opened an issue with asynqp about creating messages that lack that field, to hopefully deal with it on their end also.

@ollamh
Copy link
Contributor

ollamh commented Apr 21, 2017

@mosquito I've got the same issue when using django channels as the message sender. And I see in the official pika spec that delivery_mode can be None (pika.spec.BasicProperties https://github.com/pika/pika/blob/master/pika/spec.py#L2074).
But aside from that fact, I see that current Message class implementation assumes non-persistency for all incoming messages without delivery_mode explicitly being set. I believe the same behavior may be kept by moving default value initialization inside class initialization thus accepting None values. What do you think?

@mosquito
Copy link
Owner

What default value I should be set?

@ollamh
Copy link
Contributor

ollamh commented Apr 21, 2017

I see you already set it to non-persistent which for me is fine

@mosquito
Copy link
Owner

Ok. 😊

@ollamh
Copy link
Contributor

ollamh commented Apr 21, 2017

Great! Thank you! I can make PR if it speed up the things =)

@mosquito
Copy link
Owner

@ollamh, feel free to create PR.

@mosquito
Copy link
Owner

Fixed in 0.9.4

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

No branches or pull requests

3 participants