rabbitmq / rabbitmq-amqp1.0

AMQP 1.0 support for RabbitMQ
https://www.rabbitmq.com/
Other
93 stars 20 forks source link

crash on ambiguous omission of field in disposition frame #8

Closed mbroadst closed 9 years ago

mbroadst commented 9 years ago

I'm just putting this up to capture what we've learned on the mailing list. Proton sends a disposition frame without a "state" field which causes rabbitmq to crash on that connection with the following output (modified by @simonmacmullen):

=ERROR REPORT==== 20-Mar-2015::13:10:53 === * Generic server <0.5794.0> terminating * Last message in was {'$gen_cast', {frame, {'v1_0.disposition',true, {uint,0}, {uint,0}, true,undefined,undefined}, <0.5788.0>}}

> *\* Reason for termination == > *\* {{case_clause,undefined}, > [{rabbit_amqp1_0_session_process,'-handle_control/2-fun-2-',3,[]}, > {rabbit_amqp1_0_session,'-settle/3-fun-0-',3,[]}, > {lists,foldl,3,[{file,"lists.erl"},{line,1248}]}, > {rabbit_amqp1_0_session,settle,3,[]}, > {rabbit_amqp1_0_session_process,handle_control,2,[]}, > {rabbit_amqp1_0_session_process,handle_cast,2,[]}, > {gen_server2,handle_msg,2,[]}, > {proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]} It seems to me that rabbitmq should at very least handle this case gracefully, rather than crashing, but perhaps take an opinionated position (if the client is sending back a disposition frame in the first place, its probably an ACK right?) Cheers, Matt
mbroadst commented 9 years ago

perhaps if settled==true && state==undefined => ACK?

simonmacmullen commented 9 years ago

We shouldn't crash, sure, but I'm very wary of assuming an undefined state means ACK. I've just re-read the spec on this subject and it doesn't really give any clue.

mbroadst commented 9 years ago

Ah okay cool, I tried pretty hard this weekend to implement what you did there. Alas, my erlang-fu is incredibly weak :smile: