search

stompgem / stomp

A ruby gem for sending and receiving messages from a Stomp protocol compliant message queue. Includes: failover logic, ssl support.
http://stomp.github.com
Apache License 2.0
152 stars 80 forks source link

Explicitly flushing the connection to prevent last message chunk from not being sent #45

Closed JGailor closed 12 years ago

JGailor commented 12 years ago

Had an issue where sending even a small volume of messages would result in occasionally losing last chunk of the message being sent from the stomp client to the ActiveMQ server (explicitly last chunk of XML string was not sent, resulting in malformed XML). Ran tests with 25k messages and was able to reproduce the issue. Adding explicit flush call to the socket solved the problem over the same 25k messages.

gmallard commented 12 years ago

Hi Jeremy - Thanks for the pull request. Appreciate the effort.

This gem has been around for 6-7 years with no similar complaints. It seems imprudent to me to force the overhead of flush on all current gem users. An option to request flush would certainly be acceptable.

Perhaps this could be accomplished a little differently.......

What about something like:

:flush_on_write => true/false

with a default of false, added to a connection Hash.

Can you work up something like that?

Nice company name BTW.

JGailor commented 12 years ago

Hi Guy,

Thanks for the feedback! I think this is a great approach and I will spend some time today making the changes you suggest and writing some test specs around them.

I appreciate not wanting to force this change on anyone else, and I've wracked my brain trying to figure out why I would be seeing this problem without seeing any other issues around it on the github wiki. I have seen other references to this issue with Stomp and ActiveMQ on some other boards, but people didn't post a lot of information on how they solved this problem themselves. The only thing I was able to narrow it down to for my case was that it happened regularly at a large volume of messages (a use case the system has) and flushing the socket consistently reduced the error rate to 0%.

I will send you a pull request once I have had a chance to make the changes you suggested.

Thanks,

Jeremy

On Monday, June 18, 2012 at 5:02 PM, Guy M. Allard wrote:

Hi Jeremy - Thanks for the pull request. Appreciate the effort.

This gem has been around for 6-7 years with no similar complaints. It seems imprudent to me to force the overhead of flush on all current gem users. An option to request flush would certainly be acceptable.

Perhaps this could be accomplished a little differently.......

What about something like:

:flush_on_write => true/false

with a default of false, added to a connection Hash.

Can you work up something like that?

Nice company name BTW.

Reply to this email directly or view it on GitHub: https://github.com/stompgem/stomp/pull/45#issuecomment-6412749

gmallard commented 12 years ago

Jeremy - Looking forward to the pull request.

I am curious. In your failing test case:

a) How large are the message bodies on average? b) Are the client and AMQ on the same net? c) Has AMQ actually been 'tuned'? (Should not make a difference for this, but ....)

I run testing against AMQ, and with AMQ out of the box, totally un-tuned. But my test bed is usually on the same machine or at most 10 feet away from the server. And I very seldom run really large volume tests.

However, a flush option is a good idea. I just don't want any other gem users to come back and complain about it.

One other thought ...... maybe provide getter/setter for this option. Clients could switch it on/off on the fly if needed.

JGailor commented 12 years ago

Guy,

Wanted to take some time before getting back to you to get a good idea of answers for your questions:

a) Message sizes are between 1.5k and 2k on average b) The client and the AMQ are actually on different networks connected by VPN c) I don't have a good answer for this. This is part of an enterprise project, and the AMQ team is 1/2 a continent away and not very helpful for me.

I like the getter/setter option, but what do you think about adding a ":autoflush => false" to the set of default parameters, allow it to be set when the Client is created, and then also list it as an attr_accessor so that it can be overridden as necessary?

Thanks,

Jeremy Gailor

On Tuesday, June 19, 2012 at 12:22 PM, Guy M. Allard wrote:

Jeremy - Looking forward to the pull request.

I am curious. In your failing test case:

a) How large are the message bodies on average? b) Are the client and AMQ on the same net? c) Has AMQ actually been 'tuned'? (Should not make a difference for this, but ....)

I run testing against AMQ, and with AMQ out of the box, totally un-tuned. But my test bed is usually on the same machine or at most 10 feet away from the server. And I very seldom run really large volume tests.

However, a flush option is a good idea. I just don't want any other gem users to come back and complain about it.

One other thought ...... maybe provide getter/setter for this option. Clients could switch it on/off on the fly if needed.

Reply to this email directly or view it on GitHub: https://github.com/stompgem/stomp/pull/45#issuecomment-6434865

gmallard commented 12 years ago

Jeremy - Regarding :autoflush => false parameter, yes. Please add it.

The suggestion about getter/setter capability was meant to be in addition to a default parameter used during connect, and not a replacement for it. Sorry if that was not clear.

Thank you for thoughtful answers to those questions. Given your answer b) you may be in a fairly unique position such that the dropped data problem is triggered, where many gem users just do not see it.

I sympathize about answer c). Painful.

What version of AMQ if I may ask? LOL. Do they even tell you that?

Regards, Guy

gmallard commented 12 years ago

Jeremy - After rereading your last, yes an attr_accessor is fine in connection.rb.

I am probably going to cut a new gem version soon. One of the large clients is beginning a conversion to 1.1, and there is a little fall out from that effort that needs to be addressed.

So I'd like to ask that you get this done soon, or let me know that you lost interest .... either way is fine. Just let me know please.

Regards, Guy

JGailor commented 12 years ago

Hey Guy,

I am writing specs for the changes now. It's been a busy week at work involving some travel, and I should have a pull request ready for you tomorrow morning.

Thanks,

Jeremy Gailor

On Friday, June 22, 2012 at 4:29 PM, Guy M. Allard wrote:

Jeremy - After rereading your last, yes an attr_accessor is fine in connection.rb.

I am probably going to cut a new gem version soon. One of the large clients is beginning a conversion to 1.1, and there is a little fall out from that effort that needs to be addressed.

So I'd like to ask that you get this done soon, or let me know that you lost interest .... either way is fine. Just let me know please.

Regards, Guy

Reply to this email directly or view it on GitHub: https://github.com/stompgem/stomp/pull/45#issuecomment-6520534

gmallard commented 12 years ago

Cool. I just wanted to get this in to the next gem version. Which may happen more quickly than usual .....

Regards, Guy

JGailor commented 12 years ago

Just updated the pull request with the new changes. Let me know if anything needs to be tweaked and I can fix it tomorrow morning.

gmallard commented 12 years ago

Jeremy - Accepted, and thank you very much.

Please consider future contributions to the stomp gem. Drop by and check the issues list every once in a while. Or just ask if there are any pending tweaks that should be implemented.

Regards, Guy