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

Undefined method "method_class" for AMQ::Protocol::BodyFrame #218

Closed
alexnorthsoul opened this Issue Sep 30, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@alexnorthsoul

alexnorthsoul commented Sep 30, 2015

Hello, guys

unfortunately, there's a bug

undefined method `method_class' for #<AMQ::Protocol::BodyFrame:0x0000001e8a60b0>

ruby/2.1.0/gems/amqp-1.5.0/lib/amqp/session.rb:1112:in `frameset_complete?'
ruby/2.1.0/gems/amqp-1.5.0/lib/amqp/session.rb:932:in `receive_frame'
ruby/2.1.0/gems/amqp-1.5.0/lib/amqp/session.rb:671:in `receive_data'
ruby/2.1.0/gems/eventmachine-1.0.4/lib/eventmachine.rb:187:in `run_machine

Here's a line, which raises an exception:

first_frame.final? || (first_frame.method_class.has_content? && content_complete?(frames[1..-1]))

in case, when first_frame.final? evaluates to false.

This happens if first_frame is a HeaderFrame or a BodyFrame

@peterwilson

This comment has been minimized.

Show comment
Hide comment
@peterwilson

peterwilson Sep 30, 2015

@alexnorthsoul in what context does this happen? A BodyFrame is not supposed to be the first frame in the buffer (from my understanding). I was fighting this issue myself today.

For me, an exception was thrown on the callback block of the first message received, causing the channel's buffer to not be cleared by this line:

clear_frames_on(frame.channel) if @frames[frame.channel]

in #receive_frame. So each subsequent message received on that channel would call first_frame.method_class on the leftover BodyFrame.

I was about to submit an issue myself but just saw yours.

My thought is that #receive_frameset should have some sort of assurance of cleanup. Even if one of my messages blows out the top of the call stack, AMQP shouldn't leave itself in a bad state.

Anywhere you call user specified code without a guaranteed cleanup there can be bugs like the one outlined above.

peterwilson commented Sep 30, 2015

@alexnorthsoul in what context does this happen? A BodyFrame is not supposed to be the first frame in the buffer (from my understanding). I was fighting this issue myself today.

For me, an exception was thrown on the callback block of the first message received, causing the channel's buffer to not be cleared by this line:

clear_frames_on(frame.channel) if @frames[frame.channel]

in #receive_frame. So each subsequent message received on that channel would call first_frame.method_class on the leftover BodyFrame.

I was about to submit an issue myself but just saw yours.

My thought is that #receive_frameset should have some sort of assurance of cleanup. Even if one of my messages blows out the top of the call stack, AMQP shouldn't leave itself in a bad state.

Anywhere you call user specified code without a guaranteed cleanup there can be bugs like the one outlined above.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Oct 1, 2015

Member

@Inanepenguin's hypothesis sounds realistic

Member

michaelklishin commented Oct 1, 2015

@Inanepenguin's hypothesis sounds realistic

@peterwilson

This comment has been minimized.

Show comment
Hide comment
@peterwilson

peterwilson Oct 1, 2015

@michaelklishin any chance the "graceful cleanup" in #receive_format could make it in? I can look into trying to submit a PR, I'd have to come up to speed on the style/best practice for the repo. Like I said, I was about to open an issue myself as this just happened to me today. Very coincidental timing!

peterwilson commented Oct 1, 2015

@michaelklishin any chance the "graceful cleanup" in #receive_format could make it in? I can look into trying to submit a PR, I'd have to come up to speed on the style/best practice for the repo. Like I said, I was about to open an issue myself as this just happened to me today. Very coincidental timing!

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Oct 1, 2015

Member

Feel free to submit a PR, thank you.

On 1 oct 2015, at 11:32, Peter Wilson notifications@github.com wrote:

@michaelklishin any chance the "graceful cleanup" in #receive_format could make it in? I can look into trying to submit a PR, I'd have to come up to speed on the style/best practice for the repo. Like I said, I was about to open an issue myself as this just happened to me today. Very coincidental timing!


Reply to this email directly or view it on GitHub.

Member

michaelklishin commented Oct 1, 2015

Feel free to submit a PR, thank you.

On 1 oct 2015, at 11:32, Peter Wilson notifications@github.com wrote:

@michaelklishin any chance the "graceful cleanup" in #receive_format could make it in? I can look into trying to submit a PR, I'd have to come up to speed on the style/best practice for the repo. Like I said, I was about to open an issue myself as this just happened to me today. Very coincidental timing!


Reply to this email directly or view it on GitHub.

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Oct 17, 2016

taca
Update ruby-amqp to 1.6.0.
## Changes Between 1.5.x and 1.6.0 (Apr 4th, 2016)

### amq-protocol Update

Minimum `amq-protocol` version is now `2.0.1`.

### Provide More Details in TCP Connection Failure Exception

Contributed by Neil Hooey.

GH issue: [#222](ruby-amqp/amqp#222).


### Ensures frameset is cleared after an unhandled exception

Ensures frameset is cleared after an unhandled exception.
This avoids confusing exceptions such as

```
undefined method `method_class' for #<AMQ::Protocol::BodyFrame:0x0000001e8a60b0>
```

Contributed by Michael Lutsiuk.

GH issue: [#218](ruby-amqp/amqp#218)
@relonger

This comment has been minimized.

Show comment
Hide comment
@relonger

relonger Nov 17, 2016

The issue is still exists in gem 1.6.0 but solved in 1.5.1. Do you plan to update 1.6.x gem ?

relonger commented Nov 17, 2016

The issue is still exists in gem 1.6.0 but solved in 1.5.1. Do you plan to update 1.6.x gem ?

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Nov 17, 2016

Member

@relonger that's curious. What specific commit is in 1.5.x-stable but not master? Happy to release a new 1.6.x version later today or tomorrow.

Member

michaelklishin commented Nov 17, 2016

@relonger that's curious. What specific commit is in 1.5.x-stable but not master? Happy to release a new 1.6.x version later today or tomorrow.

@relonger

This comment has been minimized.

Show comment
Hide comment
@relonger

relonger Nov 17, 2016

This commit dd0b2de seems to solve the problem.

relonger commented Nov 17, 2016

This commit dd0b2de seems to solve the problem.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Nov 17, 2016

Member

@relonger I cherry-picked 02a9298 to master, please give it a try.

Member

michaelklishin commented Nov 17, 2016

@relonger I cherry-picked 02a9298 to master, please give it a try.

@maattdd

This comment has been minimized.

Show comment
Hide comment
@maattdd

maattdd Feb 2, 2017

Could you release a 1.6.1 with this commit included ?

maattdd commented Feb 2, 2017

Could you release a 1.6.1 with this commit included ?

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Feb 2, 2017

Member

@maattdd do you confirm that the issue is gone in master?

Member

michaelklishin commented Feb 2, 2017

@maattdd do you confirm that the issue is gone in master?

@maattdd

This comment has been minimized.

Show comment
Hide comment
@maattdd

maattdd Feb 2, 2017

@michaelklishin yes I confirm this bug is in 1.6.0 but fixed in master.

maattdd commented Feb 2, 2017

@michaelklishin yes I confirm this bug is in 1.6.0 but fixed in master.

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Feb 2, 2017

Member

@maattdd I released a 1.7.0.

Member

michaelklishin commented Feb 2, 2017

@maattdd I released a 1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment